From 9136afc67adde1b1bd63cb88bed62c6efd2455d8 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Mon, 17 Feb 2025 15:49:06 -0300
Subject: [PATCH 01/15] WIP SystemContract

---
 src/contract/CMakeLists.txt               |   2 +
 src/contract/templates/systemcontract.cpp | 386 ++++++++++++++++++++++
 src/contract/templates/systemcontract.h   | 186 +++++++++++
 3 files changed, 574 insertions(+)
 create mode 100644 src/contract/templates/systemcontract.cpp
 create mode 100644 src/contract/templates/systemcontract.h

diff --git a/src/contract/CMakeLists.txt b/src/contract/CMakeLists.txt
index 4e4805c9..a58bbd42 100644
--- a/src/contract/CMakeLists.txt
+++ b/src/contract/CMakeLists.txt
@@ -36,6 +36,7 @@ set(CONTRACT_HEADERS
   ${CMAKE_SOURCE_DIR}/src/contract/templates/dexv2/dexv2router02.h
   ${CMAKE_SOURCE_DIR}/src/contract/templates/dexv2/uq112x112.h
   ${CMAKE_SOURCE_DIR}/src/contract/templates/pebble.h
+  ${CMAKE_SOURCE_DIR}/src/contract/templates/systemcontract.h
   ${CMAKE_SOURCE_DIR}/src/contract/templates/throwtestA.h
   ${CMAKE_SOURCE_DIR}/src/contract/templates/throwtestB.h
   ${CMAKE_SOURCE_DIR}/src/contract/templates/throwtestC.h
@@ -74,6 +75,7 @@ set(CONTRACT_SOURCES
   ${CMAKE_SOURCE_DIR}/src/contract/templates/dexv2/dexv2pair.cpp
   ${CMAKE_SOURCE_DIR}/src/contract/templates/dexv2/dexv2router02.cpp
   ${CMAKE_SOURCE_DIR}/src/contract/templates/pebble.cpp
+  ${CMAKE_SOURCE_DIR}/src/contract/templates/systemcontract.cpp
   ${CMAKE_SOURCE_DIR}/src/contract/templates/throwtestA.cpp
   ${CMAKE_SOURCE_DIR}/src/contract/templates/throwtestB.cpp
   ${CMAKE_SOURCE_DIR}/src/contract/templates/throwtestC.cpp
diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
new file mode 100644
index 00000000..b1cae96a
--- /dev/null
+++ b/src/contract/templates/systemcontract.cpp
@@ -0,0 +1,386 @@
+/*
+Copyright (c) [2023-2024] [AppLayer Developers]
+
+This software is distributed under the MIT License.
+See the LICENSE.txt file in the project root for more information.
+*/
+
+#include "systemcontract.h"
+
+#include "../../utils/strconv.h"
+
+#include <set>
+
+// Tie-breaks validators with the exact same number of votes by their public key bytes
+struct ValidatorVotes {
+  PubKey    validator;
+  uint64_t  votes;
+  bool operator<(const ValidatorVotes& other) const {
+    if (votes != other.votes) {
+      return votes < other.votes;
+    }
+    for (size_t i = 0; i < 33; ++i) { // 33 = PubKey byte size
+      if (validator[i] != other.validator[i]) {
+        return validator[i] < other.validator[i];
+      }
+    }
+    return false;
+  }
+};
+
+void SystemContract::recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive) {
+  // check that the current validator votes + the current delegation delta + the new delta won't
+  // end outside of uint64_t range. if it is OK, record it in the intermediary int256_t.
+  int256_t checker = 0;
+  for (int i = 0; i < validators_.size(); ++i) {
+    // if validator not found, current delegated amount is 0, meaning no base value
+    // to consider in overflow calculation (checker == 0).
+    if (validators_[i] == validator) {
+      checker = validatorVotes_[i];
+      break;
+    }
+  }
+  // consider the vote deltas we have already accumulated in previous delegate/undelegate
+  // txs processed before in this current block
+  checker += delegationDeltas_[validator];
+  // consider the present vote delta being applied by the caller
+  if (positive) {
+    checker += delta;
+  } else {
+    checker -= delta;
+  }
+  // the resulting new ranking value for the validator must fit in uint64_t
+  if (checker > std::numeric_limits<uint64_t>::max() || checker < 0) {
+    throw DynamicException("Delegation amount limit exceeded");
+  }
+  // All OK, so record it
+  if (positive) {
+    delegationDeltas_[validator] += delta;
+  } else {
+    delegationDeltas_[validator] -= delta;
+  }
+}
+
+SystemContract::SystemContract(
+  const Address& address, const DB& db
+) : DynamicContract(address, db)
+{
+  // TODO read state from DB
+  //this->owner_ = Address(db.get(std::string("owner_"), this->getDBPrefix()));
+  //this->owner_.commit();
+
+  doRegister();
+}
+
+SystemContract::SystemContract(
+  const std::vector<PubKey>& initialValidators,
+  const uint64_t& initialNumSlots, const uint64_t& maxSlots,
+  const Address& address, const Address& creator, const uint64_t& chainId
+) : DynamicContract("SystemContract", address, creator, chainId)
+{
+  // numSlots cannot be less than the size of the initial validator set.
+  uint64_t effectiveNumSlots = std::min(initialNumSlots, initialValidators.size());
+  if (effectiveNumSlots > maxSlots) {
+    throw DynamicException("FATAL: effective validator numSlots exceeds provided maxSlots");
+  }
+  this->numSlots_ = effectiveNumSlots;
+  this->numSlots_.commit();
+  this->maxSlots_ = maxSlots;
+  this->maxSlots_.commit();
+  // The contract creator votes with 0 tokens on each initial validator key.
+  // This is the only instance where a delegation of 0 is allowed; it is needed here so that
+  //  when a default (genesis) validator is unvoted, that will cause the reimbursement of
+  //  0 tokens to the creator.
+  // The validatorVotes_ are set to 0. This can cause the validator set to choose randomly
+  //  which validators from the initial set are used to fill the remaining slots every time
+  //  the validator set is reevaluated. This is fine, since the initial validators are
+  //  supposed to be replaced as soon as possible anyway, or at least receive actual votes.
+  for (const auto& validatorPubKey : initialValidators) {
+    this->delegations_[creator][validatorPubKey] = 0;
+    this->validators_.push_back(validatorPubKey);
+    this->validatorVotes_.push_back(0);
+  }
+  this->delegations_.commit();
+  this->validators_.commit();
+  this->validatorVotes_.commit();
+
+  doRegister();
+}
+
+void SystemContract::registerContractFunctions() {
+  SystemContract::registerContract();
+  /*
+  this->registerMemberFunction("onlyOwner", &Ownable::onlyOwner, FunctionTypes::NonPayable, this);
+  this->registerMemberFunction("owner", &Ownable::owner, FunctionTypes::View, this);
+  this->registerMemberFunction("renounceOwnership", &Ownable::renounceOwnership, FunctionTypes::NonPayable, this);
+  this->registerMemberFunction("transferOwnership", &Ownable::transferOwnership, FunctionTypes::NonPayable, this);
+  */
+}
+
+// TODO/REVIEW: rewrite as solidity deposit / fallback method?
+void SystemContract::stake() {
+  // Tx native token value transferred is the staking amount, and tx sender is the depositor.
+  stakes_[this->getCaller()] += encodeAmount(this->getValue());
+}
+
+void SystemContract::unstake(const uint256_t& amount) {
+  // Tx native token value transferred is the staking amount, and tx sender is the depositor.
+  const auto& caller = this->getCaller();
+  if (stakes_.find(caller) == stakes_.end()) {
+    throw DynamicException("No stake");
+  }
+  const uint64_t amount64 = encodeAmount(amount);
+  if (amount64 == 0) {
+    throw DynamicException("Cannot unstake zero tokens");
+  }
+  if (stakes_[caller] < amount64) {
+    throw DynamicException("Insufficient balance");
+  }
+  stakes_[caller] -= amount64;
+  if (stakes_[caller] == 0) {
+    stakes_.erase(caller);
+  }
+  const uint256_t amount256 = decodeAmount(amount64);
+  this->sendTokens(caller, amount256);
+}
+
+void SystemContract::delegate(const PubKey& validator, const uint256_t& amount) {
+  const auto& caller = this->getCaller();
+  if (stakes_.find(caller) == stakes_.end()) {
+    throw DynamicException("No stake");
+  }
+  const uint64_t amount64 = encodeAmount(amount);
+  if (amount64 == 0) {
+    throw DynamicException("Cannot delegate zero tokens");
+  }
+  if (stakes_[caller] < amount64) {
+    throw DynamicException("Insufficient balance");
+  }
+  stakes_[caller] -= amount64;
+  if (stakes_[caller] == 0) {
+    stakes_.erase(caller);
+  }
+  if (delegations_.find(caller) == delegations_.end()) {
+    // It is only possible to delegate to a key if that key has a delegation to itself already.
+    // FIXME/TODO: here, deriving the address of validator must yield the caller address
+    // otherwise throw DynamicException("Unregistered validator")
+  }
+  delegations_[caller][validator] += amount64;
+  recordDelegationDelta(validator, amount64, true);
+}
+
+void SystemContract::undelegate(const PubKey& validator, const uint256_t& amount) {
+  const auto& caller = this->getCaller();
+  if (delegations_.find(caller) == delegations_.end()) {
+    throw DynamicException("No delegations");
+  }
+  if (delegations_[caller].find(validator) == delegations_[caller].end()) {
+    throw DynamicException("No delegation to validator");
+  }
+  const uint64_t amount64 = encodeAmount(amount);
+  // To undelegate initial validators (with total delegation amount == 0), the contract
+  // creator can undelegate any positive amount so this check will pass.
+  // (It is also possible to remove the initial validators if any voter account simply
+  // delegates and undelegates any positive amount).
+  if (amount64 == 0) {
+    throw DynamicException("Cannot undelegate zero tokens");
+  }
+  // If the undelegate amount is too large, then just undelegate everything
+  uint64_t effectiveAmount = std::min(delegations_[caller][validator], amount64);
+  delegations_[caller][validator] -= amount64;
+  if (delegations_[caller][validator] == 0) {
+    delegations_[caller].erase(validator);
+    if (delegations_[caller].empty()) {
+      delegations_.erase(caller);
+    }
+  }
+  stakes_[caller] += effectiveAmount;
+  recordDelegationDelta(validator, amount64, false);
+}
+
+void SystemContract::voteslots(const PubKey& validator, const uint64_t& slots) {
+  // FIXME/TODO: this->getCaller() address must match validator
+  // i.e. deriving the address from validator (pubkey) must == this->getCaller();
+  bool found = false;
+  for (int i = 0; i < numSlots_.get(); ++i) {
+    if (validators_[i] == validator) {
+      found = true;
+      break;
+    }
+  }
+  if (!found) {
+    throw DynamicException("Validator not elected");
+  }
+  if (slots < 1 || slots > maxSlots_.get()) {
+    throw DynamicException("Proposed slot count is invalid");
+  }
+  targetSlots_[validator] = slots;
+}
+
+// TODO: call this from incomingBlock()
+void SystemContract::processBlock() {
+  // feed updated validators_ & validatorVotes_ into a sorted validator,votes set
+  // save a copy of the validators_ & validatorVotes_ vector in old
+  std::set<ValidatorVotes> sorted;
+  std::vector<ValidatorVotes> old;
+  for (int i = 0; i < validators_.size(); ++i) {
+    const auto& validator = validators_[i];
+    ValidatorVotes vv;
+    vv.validator = validator;
+    vv.votes = validatorVotes_[i];
+    old.push_back(vv);
+    if (validatorVotes_[i] > 0) {
+      int256_t delta = delegationDeltas_[validator];
+      if (delta >= 0) {
+        vv.votes += delta.convert_to<uint64_t>();
+      } else {
+        delta = -delta;
+        vv.votes -= delta.convert_to<uint64_t>();
+      }
+      sorted.insert(vv);
+    }
+  }
+
+  // rebuild validators_ & validatorVotes_
+  validators_.clear();
+  validatorVotes_.clear();
+  std::set<PubKey> elected;
+  int i = 0;
+  for (const auto& vv : sorted) {
+    validators_.push_back(vv.validator);
+    validatorVotes_.push_back(vv.votes);
+    if (i < numSlots_.get()) {
+      elected.insert(vv.validator);
+    }
+    ++i;
+  }
+
+  // clear delegation deltas
+  delegationDeltas_.clear();
+
+  // clear irrelevant targetSlots_ entries created by unelected validators.
+  // NOTE: it = erase(it) is not compiling, so we do a deferred erase-by-key loop.
+  std::vector<PubKey> keysToErase;
+  auto it = targetSlots_.begin();
+  while (it != targetSlots_.end()) {
+    if (!elected.contains(it->first)) {
+      keysToErase.push_back(it->first);
+    }
+    ++it;
+  }
+  for (const auto& key : keysToErase) {
+    targetSlots_.erase(key);
+  }
+  
+  // Save oldNumSlots since numSlots_ may change
+  uint64_t oldNumSlots = numSlots_.get();
+
+  // Evaluate targetSlots_ to see if numSlots_ changes.
+  //2/3+1 of numSlots have a lower or greater number.
+  //foreach elected
+  // signed int incvote = int max
+  // signed int decvote = int min
+  // if vote > numslots   incvote = min(vote, incvote)   incvotecount++
+  // if vote < numslots   decvote = max(vote, decvote)   decvotecount++
+  // if (incvotecount > 2/3+1 of slots && incvote != numslots && incvote <= maxslots) numslots = incvote
+  // if (decvotecount > 2/3+1 of slots && decvote != numslots && decvote > 0) decslots = incvote
+
+  int64_t incVote = std::numeric_limits<int64_t>::max();
+  int64_t decVote = std::numeric_limits<int64_t>::min();
+  uint64_t incVoteCount = 0;
+  uint64_t decVoteCount = 0;
+  int vIdx = 0;
+  auto sit = sorted.begin();
+  uint64_t electedValidatorCount = 0;
+  while (sit != sorted.end() && vIdx < numSlots_.get()) {
+    ++electedValidatorCount;
+    int64_t slotsVote = static_cast<int64_t>(targetSlots_[sit->validator]); // cast is OK since slotsVote < maxSlots_
+    if (slotsVote > numSlots_.get()) {
+      incVote = std::min(incVote, slotsVote);
+      incVote = std::min(incVote, static_cast<int64_t>(maxSlots_.get())); // ensure whatever maxSlots_ is cannot be exceeded
+      ++incVoteCount;
+    } else if (slotsVote < numSlots_.get()) {
+      decVote = std::max(decVote, slotsVote);
+      // already protected against slotsVote == 0 on vote submission
+      ++decVoteCount;
+    }
+    ++sit;
+    ++vIdx;
+  }
+  uint64_t quorum = ((electedValidatorCount * 2) / 3) + 1;
+  if (incVoteCount >= quorum) {
+    numSlots_ = static_cast<uint64_t>(incVote);
+    LOGXTRACE("Increased numSlots_: " + std::to_string(numSlots_.get()));
+  } else if (decVoteCount >= quorum) {
+    numSlots_ = static_cast<uint64_t>(decVote);
+    LOGXTRACE("Decreased numSlots_: " + std::to_string(numSlots_.get()));
+  }
+
+  // Finally, compute the CometBFT validator updates
+
+  // For each validator that was elected previously, we may generate an update
+  for (int i = 0; i < old.size() && i < oldNumSlots; ++i) {
+    const auto& oldvv = old[i];
+
+    // For each old elected validator, figure out its new voting power (which is zero if its rank/index
+    //  is higher than the potentially new numSlots_ limit).
+    uint64_t newVote = 0;
+    int j = 0;
+    auto it = sorted.begin();
+    while (it != sorted.end() && j < numSlots_.get()) {
+      if (it->validator == oldvv.validator) {
+        newVote = it->votes;
+        break;
+      }
+      ++it;
+      ++j;
+    }
+
+    // If the new effective voting power for the validator change, generate an update
+    if (newVote != oldvv.votes) {
+      // TODO: return another validator update:
+      //   validator: oldvv.validator
+      //   voting power: newVote
+      LOGXTRACE("Validator update (existing): " + Hash(oldvv.validator.asBytes()).hex().get());
+    }
+  }
+
+  // Generate updates for fresh elected validators (validators that had
+  // zero votes before this block and are now elected).
+  int k = 0;
+  for (const auto& vv : sorted) {
+    if (k >= numSlots_.get()) {
+      // exceeded numSlots_, reached the voted but unelected validator range
+      break;
+    }
+    ++k;
+    bool found = false;
+    int l = 0;
+    for (const auto& oldvv : old) {
+      if (l >= oldNumSlots) {
+        // exceeded oldNumSlots, reached the voted but unelected validator range
+        break;
+      }
+      ++l;
+      if (oldvv.validator == vv.validator) {
+        found = true;
+        break;
+      }
+    }
+    if (!found) {
+      // vv.validator has >0 power now, but had ==0 power before (not elected)
+      // TODO: return another validator update
+      //   validator: validator
+      //   voting power: votes
+    }
+  }
+}
+
+DBBatch SystemContract::dump() const {
+  DBBatch batch = BaseContract::dump();
+
+  // TODO: write state to db
+  //batch.push_back(StrConv::stringToBytes("owner_"), this->owner_.get().asBytes(), this->getDBPrefix());
+
+  return batch;
+}
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
new file mode 100644
index 00000000..e8f3bf53
--- /dev/null
+++ b/src/contract/templates/systemcontract.h
@@ -0,0 +1,186 @@
+/*
+Copyright (c) [2023-2024] [AppLayer Developers]
+
+This software is distributed under the MIT License.
+See the LICENSE.txt file in the project root for more information.
+*/
+
+#ifndef OWNABLE_H
+#define OWNABLE_H
+
+#include "../dynamiccontract.h"
+#include "../variables/safeaddress.h"
+#include "../variables/safeunorderedmap.h"
+#include "../variables/safevector.h"
+#include "../variables/safeint.h"
+
+/**
+ * Chain governor contract
+ *
+ * NOTE: All token amounts are saved internally in uint64_t and multiplied by 1'000'000'000,
+ * so any amounts under 0.000000001 are kept by the contract (effectively burned).
+ *
+ * NOTE: Maximum supported voting power or delegation is ~9.2 billion tokens: it should
+ * not be possible to generate that many tokens (total) but we should do our best to
+ * protect against this (it's only a problem in the current overflow protection code if
+ * intermediary calculations exceed int256_t, so it's never a problem).
+ *
+ * TODO:
+ * - Contract class is fully written and compiles.
+ * - Register contract class / add to the contract template variant.
+ * - Auto-deploy contract on initChain() (should be a fixed contract address)
+ *   has to be initChain() since that's the one that gets the genesis file.
+ * - Hook Blockchain::incomingBlock() to the system contract.
+ *   - normal calls to the contract generate data structure changes
+ *   - the newValidators is collected by incomingBlock() at its end, if any
+ *     by calling the unregistered contract method directly.
+ * - Write cometValidatorUpdates to a DB prefix in the blocks db.
+ *   bytes33 pub key + uint64_t value which is the score
+ * - Write unit test.
+ * - PR branch.
+ */
+class SystemContract : public DynamicContract {
+  private:
+    void registerContractFunctions() override; ///< Register the contract functions.
+
+    void doRegister() {
+      SystemContract::registerContractFunctions();
+      this->numSlots_.enableRegister();
+      this->maxSlots_.enableRegister();
+      this->targetSlots_.enableRegister();
+      this->stakes_.enableRegister();
+      this->delegations_.enableRegister();
+      this->validators_.enableRegister();
+      this->validatorVotes_.enableRegister();
+      this->delegationDeltas_.enableRegister();
+    }
+
+    static constexpr uint64_t AMOUNT_ENCODING_SCALE = 1000000000ULL; // 1e9
+
+    /// Encode a 256-bit amount to 64 bits
+    static uint64_t encodeAmount(const uint256_t &amountWei) {
+      uint256_t remainder = amountWei % AMOUNT_ENCODING_SCALE;
+      if (remainder != 0) {
+        throw DynamicException("Cannot send dust to the system contract");
+      }
+      uint256_t quotient = amountWei / AMOUNT_ENCODING_SCALE;
+      if (quotient > std::numeric_limits<uint64_t>::max()) {
+        throw DynamicException("System contract deposit too large");
+      }
+      return static_cast<uint64_t>(quotient);
+    }
+
+    /// Decode a 256-bit amount from 64 bits
+    static uint256_t decodeAmount(uint64_t amount64) {
+      return uint256_t(amount64) * AMOUNT_ENCODING_SCALE;
+    }
+
+    void recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive);
+  
+    SafeUint64_t numSlots_; /// The active number of validator slots.
+    SafeUint64_t maxSlots_; /// The maximum value for numSlots_.
+
+    /// Elected validators' votes on the future number of validator slots.
+    /// If a number different to numSlots_ is agreed by >2/3 of votes in
+    /// targetSlots_, then that number will be the new numSlots_.
+    SafeUnorderedMap<PubKey, uint64_t> targetSlots_;
+
+    /// Liquid deposits by users.
+    SafeUnorderedMap<Address, uint64_t> stakes_;
+
+    /// Validator delegations (votes) by users.
+    /// Maps voter address to delegations, where each delegation is a validator key
+    /// and a 64-bit encoded delegation (vote) amount.
+    SafeUnorderedMap<Address, boost::unordered_flat_map<PubKey, uint64_t>> delegations_;
+
+    /// Sorted list of all validator candidates; the first numSlots_ elements are the elected validators.
+    SafeVector<PubKey> validators_;
+
+    /// Sorted list of all validator candidate vote totals.
+    /// Indices in validatorVotes_ match indices in validators_.
+    /// For simplicity, it is just not allowed for two validators to have the exact same amount of votes,
+    /// so any inbound vote that will cause a value collision will loop decreasing the delegated amount by
+    /// 1 unit until the collision is resolved (or the delegation fails with an amount of 0).
+    SafeVector<uint64_t> validatorVotes_;
+
+    /// List of queued validator delegation changes (increases/decreases in votes).
+    /// Delegation (vote) deltas are accumulated during block (tx) processing and applied
+    /// after all txs in a block are processed, generating the new validator ranking and
+    /// thus the needed validator updates for CometBFT.
+    SafeUnorderedMap<PubKey, int256_t> delegationDeltas_;
+
+  public:
+    using ConstructorArguments = std::tuple<
+      std::vector<PubKey>&,
+      const uint64_t&,
+      const uint64_t&
+    >;
+
+    /**
+     * Constructor for loading contract from DB.
+     * @param address The address of the contract.
+     * @param db The database to use.
+     */
+    SystemContract(const Address& address, const DB& db);
+
+    /**
+     * Create the system contract.
+     * This constructor is invoked by the node itself (class Blockchain) to create exactly
+     * one instance of this contract at genesis.
+     * @param initialValidators The initial validator set. The voting power assigned on genesis
+     * is irrelevant as the delegation amount will be set to zero by a dummy voter address.
+     * @param initialNumSlots The initial number of validator slots.
+     * @param maxSlots The maximum number of validator slots.
+     * @param address The address of the contract.
+     * @param creator The address of the creator of the contract.
+     * @param chainId The chain ID.
+     */
+    SystemContract(
+      const std::vector<PubKey>& initialValidators,
+      const uint64_t& initialNumSlots, const uint64_t& maxSlots,
+      const Address& address, const Address& creator, const uint64_t& chainId
+    );
+
+    /// Deposit native tokens
+    void stake();
+
+    /// Withdraw native tokens
+    void unstake(const uint256_t& amount);
+
+    /// Vote for a validator
+    void delegate(const PubKey& validator, const uint256_t& amount);
+
+    /// Unvote a validator
+    void undelegate(const PubKey& validator, const uint256_t& amount);
+
+    /// Validator changes a slots vote
+    /// The caller address must match the validator public key
+    void voteslots(const PubKey& validator, const uint64_t& slots);
+
+    /// Apply processing at the end of a block.
+    /// This is an unregistered function called directly by the block processor
+    /// after all block txs are applied to the machine state.
+    void processBlock();
+
+    /// Register the contract.
+    static void registerContract() {
+      // TODO: methods
+      ContractReflectionInterface::registerContractMethods<SystemContract>(
+        std::vector<std::string>{"initialValidators", "initialNumSlots", "maxSlots"}
+        // stake
+        // unstake
+        // deletate
+        // undelegate
+        // voteslots
+      );
+
+      // REVIEW: should we add events for validator set updates generated and numSlots changes?
+      //         I guess it wouldn't hurt to have those. Also helps testing (?)
+      //ContractReflectionInterface::registerContractEvents<SystemContract>(
+      //);
+    }
+
+    DBBatch dump() const override; ///< Dump method.
+};
+
+#endif // OWNABLE_H

From 502f6db8dd078bd0436c6c180d6dbed05ec7096d Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Mon, 17 Feb 2025 16:15:17 -0300
Subject: [PATCH 02/15] Enforce unique delegation amounts for all validators

---
 src/contract/templates/systemcontract.cpp | 46 +++++++++++++++--------
 src/contract/templates/systemcontract.h   |  4 +-
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index b1cae96a..346da1a6 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -28,7 +28,7 @@ struct ValidatorVotes {
   }
 };
 
-void SystemContract::recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive) {
+bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive) {
   // check that the current validator votes + the current delegation delta + the new delta won't
   // end outside of uint64_t range. if it is OK, record it in the intermediary int256_t.
   int256_t checker = 0;
@@ -53,12 +53,22 @@ void SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
   if (checker > std::numeric_limits<uint64_t>::max() || checker < 0) {
     throw DynamicException("Delegation amount limit exceeded");
   }
+  uint64_t targetVotes = checker.convert_to<uint64_t>();
+  // No validator can have exactly the same voting power as any other
+  // If we are violating this rule with the new voting power, return false so the
+  //   caller can retry with another amount
+  for (int i = 0; i < validatorVotes_.size(); ++i) {
+    if (validatorVotes_[i] == targetVotes) {
+      return false;
+    }
+  }
   // All OK, so record it
   if (positive) {
     delegationDeltas_[validator] += delta;
   } else {
     delegationDeltas_[validator] -= delta;
   }
+  return true;
 }
 
 SystemContract::SystemContract(
@@ -149,7 +159,7 @@ void SystemContract::delegate(const PubKey& validator, const uint256_t& amount)
   if (stakes_.find(caller) == stakes_.end()) {
     throw DynamicException("No stake");
   }
-  const uint64_t amount64 = encodeAmount(amount);
+  uint64_t amount64 = encodeAmount(amount);
   if (amount64 == 0) {
     throw DynamicException("Cannot delegate zero tokens");
   }
@@ -166,7 +176,14 @@ void SystemContract::delegate(const PubKey& validator, const uint256_t& amount)
     // otherwise throw DynamicException("Unregistered validator")
   }
   delegations_[caller][validator] += amount64;
-  recordDelegationDelta(validator, amount64, true);
+  // Loop avoiding collision in delegation amount (which we do not support)
+  while (!recordDelegationDelta(validator, amount64, true)) {
+    if (amount64 == 1) {
+      throw DynamicException("Cannot delegate this amount (try a larger amount)");
+    }
+    --amount64;
+    delegations_[caller][validator] -= 1;
+  }
 }
 
 void SystemContract::undelegate(const PubKey& validator, const uint256_t& amount) {
@@ -177,7 +194,7 @@ void SystemContract::undelegate(const PubKey& validator, const uint256_t& amount
   if (delegations_[caller].find(validator) == delegations_[caller].end()) {
     throw DynamicException("No delegation to validator");
   }
-  const uint64_t amount64 = encodeAmount(amount);
+  uint64_t amount64 = encodeAmount(amount);
   // To undelegate initial validators (with total delegation amount == 0), the contract
   // creator can undelegate any positive amount so this check will pass.
   // (It is also possible to remove the initial validators if any voter account simply
@@ -195,7 +212,14 @@ void SystemContract::undelegate(const PubKey& validator, const uint256_t& amount
     }
   }
   stakes_[caller] += effectiveAmount;
-  recordDelegationDelta(validator, amount64, false);
+  // Loop avoiding collision in delegation amount (which we do not support)
+  while (!recordDelegationDelta(validator, amount64, false)) {
+    if (amount64 == 1) {
+      throw DynamicException("Cannot undelegate this amount (try a larger amount)");
+    }
+    --amount64;
+    delegations_[caller][validator] += 1;
+  }
 }
 
 void SystemContract::voteslots(const PubKey& validator, const uint64_t& slots) {
@@ -271,20 +295,11 @@ void SystemContract::processBlock() {
   for (const auto& key : keysToErase) {
     targetSlots_.erase(key);
   }
-  
+
   // Save oldNumSlots since numSlots_ may change
   uint64_t oldNumSlots = numSlots_.get();
 
   // Evaluate targetSlots_ to see if numSlots_ changes.
-  //2/3+1 of numSlots have a lower or greater number.
-  //foreach elected
-  // signed int incvote = int max
-  // signed int decvote = int min
-  // if vote > numslots   incvote = min(vote, incvote)   incvotecount++
-  // if vote < numslots   decvote = max(vote, decvote)   decvotecount++
-  // if (incvotecount > 2/3+1 of slots && incvote != numslots && incvote <= maxslots) numslots = incvote
-  // if (decvotecount > 2/3+1 of slots && decvote != numslots && decvote > 0) decslots = incvote
-
   int64_t incVote = std::numeric_limits<int64_t>::max();
   int64_t decVote = std::numeric_limits<int64_t>::min();
   uint64_t incVoteCount = 0;
@@ -372,6 +387,7 @@ void SystemContract::processBlock() {
       // TODO: return another validator update
       //   validator: validator
       //   voting power: votes
+      LOGXTRACE("Validator update (new): " + Hash(vv.validator.asBytes()).hex().get());
     }
   }
 }
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index e8f3bf53..b163e53c 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -75,8 +75,8 @@ class SystemContract : public DynamicContract {
       return uint256_t(amount64) * AMOUNT_ENCODING_SCALE;
     }
 
-    void recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive);
-  
+    bool recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive);
+
     SafeUint64_t numSlots_; /// The active number of validator slots.
     SafeUint64_t maxSlots_; /// The maximum value for numSlots_.
 

From cbb5cb3ce2c30c6a81d26ff8ff7d90526c2a5def Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Mon, 17 Feb 2025 16:31:16 -0300
Subject: [PATCH 03/15] skip validator set changes if no delegation or slot
 changes

---
 src/contract/templates/systemcontract.cpp | 183 ++++++++++++----------
 1 file changed, 98 insertions(+), 85 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index 346da1a6..b4858221 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -254,52 +254,59 @@ void SystemContract::processBlock() {
     vv.votes = validatorVotes_[i];
     old.push_back(vv);
     if (validatorVotes_[i] > 0) {
-      int256_t delta = delegationDeltas_[validator];
-      if (delta >= 0) {
-        vv.votes += delta.convert_to<uint64_t>();
-      } else {
-        delta = -delta;
-        vv.votes -= delta.convert_to<uint64_t>();
+      auto it = delegationDeltas_.find(validator);
+      if (it != delegationDeltas_.end()) {
+        int256_t delta = it->second;
+        if (delta >= 0) {
+          vv.votes += delta.convert_to<uint64_t>();
+        } else {
+          delta = -delta;
+          vv.votes -= delta.convert_to<uint64_t>();
+        }
       }
       sorted.insert(vv);
     }
   }
 
-  // rebuild validators_ & validatorVotes_
-  validators_.clear();
-  validatorVotes_.clear();
-  std::set<PubKey> elected;
-  int i = 0;
-  for (const auto& vv : sorted) {
-    validators_.push_back(vv.validator);
-    validatorVotes_.push_back(vv.votes);
-    if (i < numSlots_.get()) {
-      elected.insert(vv.validator);
+  bool changedDelegations = !delegationDeltas_.empty();
+
+  // If delegations are unchanged, no need to fix validators_ & validatorVotes_
+  if (changedDelegations) {
+
+    // rebuild validators_ & validatorVotes_
+    validators_.clear();
+    validatorVotes_.clear();
+    std::set<PubKey> elected;
+    int i = 0;
+    for (const auto& vv : sorted) {
+      validators_.push_back(vv.validator);
+      validatorVotes_.push_back(vv.votes);
+      if (i < numSlots_.get()) {
+        elected.insert(vv.validator);
+      }
+      ++i;
     }
-    ++i;
-  }
 
-  // clear delegation deltas
-  delegationDeltas_.clear();
+    // clear delegation deltas
+    delegationDeltas_.clear();
 
-  // clear irrelevant targetSlots_ entries created by unelected validators.
-  // NOTE: it = erase(it) is not compiling, so we do a deferred erase-by-key loop.
-  std::vector<PubKey> keysToErase;
-  auto it = targetSlots_.begin();
-  while (it != targetSlots_.end()) {
-    if (!elected.contains(it->first)) {
-      keysToErase.push_back(it->first);
+    // clear irrelevant targetSlots_ entries created by unelected validators.
+    // NOTE: it = erase(it) is not compiling, so we do a deferred erase-by-key loop.
+    std::vector<PubKey> keysToErase;
+    auto it = targetSlots_.begin();
+    while (it != targetSlots_.end()) {
+      if (!elected.contains(it->first)) {
+        keysToErase.push_back(it->first);
+      }
+      ++it;
+    }
+    for (const auto& key : keysToErase) {
+      targetSlots_.erase(key);
     }
-    ++it;
-  }
-  for (const auto& key : keysToErase) {
-    targetSlots_.erase(key);
   }
 
-  // Save oldNumSlots since numSlots_ may change
-  uint64_t oldNumSlots = numSlots_.get();
-
-  // Evaluate targetSlots_ to see if numSlots_ changes.
+  // Fully reevaluate targetSlots_ to see if numSlots_ changes.
+  uint64_t oldNumSlots = numSlots_.get(); // Save oldNumSlots since numSlots_ may change
   int64_t incVote = std::numeric_limits<int64_t>::max();
   int64_t decVote = std::numeric_limits<int64_t>::min();
   uint64_t incVoteCount = 0;
@@ -307,6 +314,7 @@ void SystemContract::processBlock() {
   int vIdx = 0;
   auto sit = sorted.begin();
   uint64_t electedValidatorCount = 0;
+  bool changedSlots = false;
   while (sit != sorted.end() && vIdx < numSlots_.get()) {
     ++electedValidatorCount;
     int64_t slotsVote = static_cast<int64_t>(targetSlots_[sit->validator]); // cast is OK since slotsVote < maxSlots_
@@ -323,71 +331,76 @@ void SystemContract::processBlock() {
     ++vIdx;
   }
   uint64_t quorum = ((electedValidatorCount * 2) / 3) + 1;
-  if (incVoteCount >= quorum) {
+  if (incVoteCount >= quorum && incVote != numSlots_.get()) {
     numSlots_ = static_cast<uint64_t>(incVote);
+    changedSlots = true;
     LOGXTRACE("Increased numSlots_: " + std::to_string(numSlots_.get()));
-  } else if (decVoteCount >= quorum) {
+  } else if (decVoteCount >= quorum && decVote != numSlots_.get()) {
     numSlots_ = static_cast<uint64_t>(decVote);
+    changedSlots = true;
     LOGXTRACE("Decreased numSlots_: " + std::to_string(numSlots_.get()));
   }
 
-  // Finally, compute the CometBFT validator updates
+  // Finally, compute the CometBFT validator updates if any.
+  // If neither delegations nor slots changed, then no validator set changes are possible.
+  if (changedSlots || changedDelegations) {
 
-  // For each validator that was elected previously, we may generate an update
-  for (int i = 0; i < old.size() && i < oldNumSlots; ++i) {
-    const auto& oldvv = old[i];
+    // For each validator that was elected previously, we may generate an update
+    for (int i = 0; i < old.size() && i < oldNumSlots; ++i) {
+      const auto& oldvv = old[i];
 
-    // For each old elected validator, figure out its new voting power (which is zero if its rank/index
-    //  is higher than the potentially new numSlots_ limit).
-    uint64_t newVote = 0;
-    int j = 0;
-    auto it = sorted.begin();
-    while (it != sorted.end() && j < numSlots_.get()) {
-      if (it->validator == oldvv.validator) {
-        newVote = it->votes;
-        break;
+      // For each old elected validator, figure out its new voting power (which is zero if its rank/index
+      //  is higher than the potentially new numSlots_ limit).
+      uint64_t newVote = 0;
+      int j = 0;
+      auto it = sorted.begin();
+      while (it != sorted.end() && j < numSlots_.get()) {
+        if (it->validator == oldvv.validator) {
+          newVote = it->votes;
+          break;
+        }
+        ++it;
+        ++j;
       }
-      ++it;
-      ++j;
-    }
 
-    // If the new effective voting power for the validator change, generate an update
-    if (newVote != oldvv.votes) {
-      // TODO: return another validator update:
-      //   validator: oldvv.validator
-      //   voting power: newVote
-      LOGXTRACE("Validator update (existing): " + Hash(oldvv.validator.asBytes()).hex().get());
+      // If the new effective voting power for the validator change, generate an update
+      if (newVote != oldvv.votes) {
+        // TODO: return another validator update:
+        //   validator: oldvv.validator
+        //   voting power: newVote
+        LOGXTRACE("Validator update (existing): " + Hash(oldvv.validator.asBytes()).hex().get());
+      }
     }
-  }
 
-  // Generate updates for fresh elected validators (validators that had
-  // zero votes before this block and are now elected).
-  int k = 0;
-  for (const auto& vv : sorted) {
-    if (k >= numSlots_.get()) {
-      // exceeded numSlots_, reached the voted but unelected validator range
-      break;
-    }
-    ++k;
-    bool found = false;
-    int l = 0;
-    for (const auto& oldvv : old) {
-      if (l >= oldNumSlots) {
-        // exceeded oldNumSlots, reached the voted but unelected validator range
+    // Generate updates for fresh elected validators (validators that had
+    // zero votes before this block and are now elected).
+    int k = 0;
+    for (const auto& vv : sorted) {
+      if (k >= numSlots_.get()) {
+        // exceeded numSlots_, reached the voted but unelected validator range
         break;
       }
-      ++l;
-      if (oldvv.validator == vv.validator) {
-        found = true;
-        break;
+      ++k;
+      bool found = false;
+      int l = 0;
+      for (const auto& oldvv : old) {
+        if (l >= oldNumSlots) {
+          // exceeded oldNumSlots, reached the voted but unelected validator range
+          break;
+        }
+        ++l;
+        if (oldvv.validator == vv.validator) {
+          found = true;
+          break;
+        }
+      }
+      if (!found) {
+        // vv.validator has >0 power now, but had ==0 power before (not elected)
+        // TODO: return another validator update
+        //   validator: validator
+        //   voting power: votes
+        LOGXTRACE("Validator update (new): " + Hash(vv.validator.asBytes()).hex().get());
       }
-    }
-    if (!found) {
-      // vv.validator has >0 power now, but had ==0 power before (not elected)
-      // TODO: return another validator update
-      //   validator: validator
-      //   voting power: votes
-      LOGXTRACE("Validator update (new): " + Hash(vv.validator.asBytes()).hex().get());
     }
   }
 }

From e46ebe1e48bada4d0b55653b54b2dd851a720e75 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Tue, 18 Feb 2025 08:35:16 -0300
Subject: [PATCH 04/15] fix & finish SystemContract interface

---
 src/contract/customcontracts.h            |  6 ++-
 src/contract/templates/systemcontract.cpp | 48 ++++++++++++++---------
 src/contract/templates/systemcontract.h   | 37 +++++++++--------
 3 files changed, 55 insertions(+), 36 deletions(-)

diff --git a/src/contract/customcontracts.h b/src/contract/customcontracts.h
index c0cc2685..29a028fa 100644
--- a/src/contract/customcontracts.h
+++ b/src/contract/customcontracts.h
@@ -23,19 +23,21 @@ See the LICENSE.txt file in the project root for more information.
 #include "templates/snailtraceroptimized.h"
 #include "templates/ownable.h"
 #include "templates/pebble.h"
+#include "templates/systemcontract.h"
 
 /// Typedef for the blockchain's registered contracts.
 #ifdef BUILD_TESTNET
 /// Typedef for the blockchain's registered contracts in TESTNET mode.
 using ContractTypes = std::tuple<
-  ERC20, NativeWrapper, DEXV2Pair, DEXV2Factory, DEXV2Router02, ERC721, ERC721URIStorage, Ownable, Pebble
+  ERC20, NativeWrapper, DEXV2Pair, DEXV2Factory, DEXV2Router02, ERC721, ERC721URIStorage,
+  Ownable, Pebble, SystemContract
 >;
 #else
 /// Typedef for the blockchain's registered contracts in normal mode.
 using ContractTypes = std::tuple<
   ERC20, ERC20Wrapper, NativeWrapper, SimpleContract, DEXV2Pair, DEXV2Factory,
   DEXV2Router02, ERC721, ThrowTestA, ThrowTestB, ThrowTestC, ERC721Test, TestThrowVars,
-  RandomnessTest, SnailTracer, SnailTracerOptimized, Pebble
+  RandomnessTest, SnailTracer, SnailTracerOptimized, Pebble, SystemContract
 >;
 #endif
 
diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index b4858221..d622dfa5 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -11,6 +11,14 @@ See the LICENSE.txt file in the project root for more information.
 
 #include <set>
 
+PubKey SystemContract::pubKeyFromString(const std::string& pubKeyStr) {
+  Bytes pubKeyBytes = Hex::toBytes(pubKeyStr);
+  if (pubKeyBytes.size() != 33) {
+    throw DynamicException("Invalid PubKey");
+  }
+  return PubKey(pubKeyBytes);
+}
+
 // Tie-breaks validators with the exact same number of votes by their public key bytes
 struct ValidatorVotes {
   PubKey    validator;
@@ -30,7 +38,7 @@ struct ValidatorVotes {
 
 bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive) {
   // check that the current validator votes + the current delegation delta + the new delta won't
-  // end outside of uint64_t range. if it is OK, record it in the intermediary int256_t.
+  // end outside of uint64_t range. if OK, save as intermediary int256_t at delegationDeltas_.
   int256_t checker = 0;
   for (int i = 0; i < validators_.size(); ++i) {
     // if validator not found, current delegated amount is 0, meaning no base value
@@ -83,11 +91,15 @@ SystemContract::SystemContract(
 }
 
 SystemContract::SystemContract(
-  const std::vector<PubKey>& initialValidators,
+  const std::vector<std::string>& initialValidatorPubKeys,
   const uint64_t& initialNumSlots, const uint64_t& maxSlots,
   const Address& address, const Address& creator, const uint64_t& chainId
 ) : DynamicContract("SystemContract", address, creator, chainId)
 {
+  std::vector<PubKey> initialValidators;
+  for (const auto& pubKeyStr : initialValidatorPubKeys) {
+    initialValidators.push_back(pubKeyFromString(pubKeyStr));
+  }
   // numSlots cannot be less than the size of the initial validator set.
   uint64_t effectiveNumSlots = std::min(initialNumSlots, initialValidators.size());
   if (effectiveNumSlots > maxSlots) {
@@ -119,12 +131,11 @@ SystemContract::SystemContract(
 
 void SystemContract::registerContractFunctions() {
   SystemContract::registerContract();
-  /*
-  this->registerMemberFunction("onlyOwner", &Ownable::onlyOwner, FunctionTypes::NonPayable, this);
-  this->registerMemberFunction("owner", &Ownable::owner, FunctionTypes::View, this);
-  this->registerMemberFunction("renounceOwnership", &Ownable::renounceOwnership, FunctionTypes::NonPayable, this);
-  this->registerMemberFunction("transferOwnership", &Ownable::transferOwnership, FunctionTypes::NonPayable, this);
-  */
+  this->registerMemberFunction("stake", &SystemContract::stake, FunctionTypes::Payable, this);
+  this->registerMemberFunction("unstake", &SystemContract::unstake, FunctionTypes::NonPayable, this);
+  this->registerMemberFunction("delegate", &SystemContract::delegate, FunctionTypes::NonPayable, this);
+  this->registerMemberFunction("undelegate", &SystemContract::undelegate, FunctionTypes::NonPayable, this);
+  this->registerMemberFunction("voteSlots", &SystemContract::voteSlots, FunctionTypes::NonPayable, this);
 }
 
 // TODO/REVIEW: rewrite as solidity deposit / fallback method?
@@ -154,7 +165,8 @@ void SystemContract::unstake(const uint256_t& amount) {
   this->sendTokens(caller, amount256);
 }
 
-void SystemContract::delegate(const PubKey& validator, const uint256_t& amount) {
+void SystemContract::delegate(const std::string& validatorPubKey, const uint256_t& amount) {
+  PubKey validator = pubKeyFromString(validatorPubKey);
   const auto& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
     throw DynamicException("No stake");
@@ -186,7 +198,8 @@ void SystemContract::delegate(const PubKey& validator, const uint256_t& amount)
   }
 }
 
-void SystemContract::undelegate(const PubKey& validator, const uint256_t& amount) {
+void SystemContract::undelegate(const std::string& validatorPubKey, const uint256_t& amount) {
+  PubKey validator = pubKeyFromString(validatorPubKey);
   const auto& caller = this->getCaller();
   if (delegations_.find(caller) == delegations_.end()) {
     throw DynamicException("No delegations");
@@ -222,7 +235,8 @@ void SystemContract::undelegate(const PubKey& validator, const uint256_t& amount
   }
 }
 
-void SystemContract::voteslots(const PubKey& validator, const uint64_t& slots) {
+void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_t& slots) {
+  PubKey validator = pubKeyFromString(validatorPubKey);
   // FIXME/TODO: this->getCaller() address must match validator
   // i.e. deriving the address from validator (pubkey) must == this->getCaller();
   bool found = false;
@@ -242,7 +256,9 @@ void SystemContract::voteslots(const PubKey& validator, const uint64_t& slots) {
 }
 
 // TODO: call this from incomingBlock()
-void SystemContract::processBlock() {
+void SystemContract::processBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas) {
+  validatorDeltas.clear();
+
   // feed updated validators_ & validatorVotes_ into a sorted validator,votes set
   // save a copy of the validators_ & validatorVotes_ vector in old
   std::set<ValidatorVotes> sorted;
@@ -365,10 +381,8 @@ void SystemContract::processBlock() {
 
       // If the new effective voting power for the validator change, generate an update
       if (newVote != oldvv.votes) {
-        // TODO: return another validator update:
-        //   validator: oldvv.validator
-        //   voting power: newVote
         LOGXTRACE("Validator update (existing): " + Hash(oldvv.validator.asBytes()).hex().get());
+        validatorDeltas.push_back({oldvv.validator, newVote});
       }
     }
 
@@ -396,10 +410,8 @@ void SystemContract::processBlock() {
       }
       if (!found) {
         // vv.validator has >0 power now, but had ==0 power before (not elected)
-        // TODO: return another validator update
-        //   validator: validator
-        //   voting power: votes
         LOGXTRACE("Validator update (new): " + Hash(vv.validator.asBytes()).hex().get());
+        validatorDeltas.push_back({vv.validator, vv.votes});
       }
     }
   }
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index b163e53c..d16a30f8 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -5,8 +5,8 @@ This software is distributed under the MIT License.
 See the LICENSE.txt file in the project root for more information.
 */
 
-#ifndef OWNABLE_H
-#define OWNABLE_H
+#ifndef SYSTEMCONTRACT_H
+#define SYSTEMCONTRACT_H
 
 #include "../dynamiccontract.h"
 #include "../variables/safeaddress.h"
@@ -75,6 +75,8 @@ class SystemContract : public DynamicContract {
       return uint256_t(amount64) * AMOUNT_ENCODING_SCALE;
     }
 
+    static PubKey pubKeyFromString(const std::string& pubKeyStr);
+
     bool recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive);
 
     SafeUint64_t numSlots_; /// The active number of validator slots.
@@ -107,11 +109,14 @@ class SystemContract : public DynamicContract {
     /// Delegation (vote) deltas are accumulated during block (tx) processing and applied
     /// after all txs in a block are processed, generating the new validator ranking and
     /// thus the needed validator updates for CometBFT.
+    /// This is int256_t because we need the delta to be an uint64_t in magnitude in either
+    /// direction (positive or negative). Could be int128_t as well, but int256_t matches
+    /// the intermediary type used for calculating and validating the delta.
     SafeUnorderedMap<PubKey, int256_t> delegationDeltas_;
 
   public:
     using ConstructorArguments = std::tuple<
-      std::vector<PubKey>&,
+      std::vector<std::string>&,
       const uint64_t&,
       const uint64_t&
     >;
@@ -127,7 +132,7 @@ class SystemContract : public DynamicContract {
      * Create the system contract.
      * This constructor is invoked by the node itself (class Blockchain) to create exactly
      * one instance of this contract at genesis.
-     * @param initialValidators The initial validator set. The voting power assigned on genesis
+     * @param initialValidatorPubKeys The initial validator set. The voting power assigned on genesis
      * is irrelevant as the delegation amount will be set to zero by a dummy voter address.
      * @param initialNumSlots The initial number of validator slots.
      * @param maxSlots The maximum number of validator slots.
@@ -136,7 +141,7 @@ class SystemContract : public DynamicContract {
      * @param chainId The chain ID.
      */
     SystemContract(
-      const std::vector<PubKey>& initialValidators,
+      const std::vector<std::string>& initialValidatorPubKeys,
       const uint64_t& initialNumSlots, const uint64_t& maxSlots,
       const Address& address, const Address& creator, const uint64_t& chainId
     );
@@ -148,30 +153,30 @@ class SystemContract : public DynamicContract {
     void unstake(const uint256_t& amount);
 
     /// Vote for a validator
-    void delegate(const PubKey& validator, const uint256_t& amount);
+    void delegate(const std::string& validatorPubKey, const uint256_t& amount);
 
     /// Unvote a validator
-    void undelegate(const PubKey& validator, const uint256_t& amount);
+    void undelegate(const std::string& validatorPubKey, const uint256_t& amount);
 
     /// Validator changes a slots vote
     /// The caller address must match the validator public key
-    void voteslots(const PubKey& validator, const uint64_t& slots);
+    void voteSlots(const std::string& validatorPubKey, const uint64_t& slots);
 
     /// Apply processing at the end of a block.
     /// This is an unregistered function called directly by the block processor
     /// after all block txs are applied to the machine state.
-    void processBlock();
+    void processBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas);
 
     /// Register the contract.
     static void registerContract() {
       // TODO: methods
       ContractReflectionInterface::registerContractMethods<SystemContract>(
-        std::vector<std::string>{"initialValidators", "initialNumSlots", "maxSlots"}
-        // stake
-        // unstake
-        // deletate
-        // undelegate
-        // voteslots
+        std::vector<std::string>{"initialValidators", "initialNumSlots", "maxSlots"},
+        std::make_tuple("stake", &SystemContract::stake, FunctionTypes::Payable, std::vector<std::string>{}),
+        std::make_tuple("unstake", &SystemContract::unstake, FunctionTypes::NonPayable, std::vector<std::string>{"amount"}),
+        std::make_tuple("delegate", &SystemContract::delegate, FunctionTypes::NonPayable, std::vector<std::string>{"validatorPubKey", "amount"}),
+        std::make_tuple("undelegate", &SystemContract::undelegate, FunctionTypes::NonPayable, std::vector<std::string>{"validatorPubKey", "amount"}),
+        std::make_tuple("voteSlots", &SystemContract::voteSlots, FunctionTypes::NonPayable, std::vector<std::string>{"validatorPubKey", "slots"})
       );
 
       // REVIEW: should we add events for validator set updates generated and numSlots changes?
@@ -183,4 +188,4 @@ class SystemContract : public DynamicContract {
     DBBatch dump() const override; ///< Dump method.
 };
 
-#endif // OWNABLE_H
+#endif // SYSTEMCONTRACT_H

From fa1e63f6c4db0560ffb3b210a4c49a6c3f021b2b Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Tue, 18 Feb 2025 10:13:50 -0300
Subject: [PATCH 05/15] system contract save/load state

---
 src/contract/templates/systemcontract.cpp | 62 ++++++++++++++++++++---
 src/contract/templates/systemcontract.h   |  6 +--
 2 files changed, 57 insertions(+), 11 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index d622dfa5..6c7d54da 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -83,10 +83,33 @@ SystemContract::SystemContract(
   const Address& address, const DB& db
 ) : DynamicContract(address, db)
 {
-  // TODO read state from DB
-  //this->owner_ = Address(db.get(std::string("owner_"), this->getDBPrefix()));
-  //this->owner_.commit();
-
+  this->numSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("numSlots_"), this->getDBPrefix()));
+  this->numSlots_.commit();
+  this->maxSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("maxSlots_"), this->getDBPrefix()));
+  this->maxSlots_.commit();
+  for (const auto& dbEntry : db.getBatch(this->getNewPrefix("targetSlots_"))) {
+    this->targetSlots_[PubKey(dbEntry.key)] = Utils::fromBigEndian<uint64_t>(dbEntry.value);
+  }
+  this->targetSlots_.commit();
+  for (const auto& dbEntry : db.getBatch(this->getNewPrefix("stakes_"))) {
+    this->stakes_[Address(dbEntry.key)] = Utils::fromBigEndian<uint64_t>(dbEntry.value);
+  }
+  this->stakes_.commit();
+  for (const auto& dbEntry : db.getBatch(this->getNewPrefix("delegations_"))) {
+    View<Bytes> valueView(dbEntry.value);
+    this->delegations_[Address(dbEntry.key)][PubKey(valueView.subspan(0, 33))] = Utils::fromBigEndian<uint64_t>(valueView.subspan(33));
+  }
+  this->delegations_.commit();
+  for (const auto& dbEntry : db.getBatch(this->getNewPrefix("validators_"))) {
+    this->validators_.push_back(PubKey(dbEntry.value));
+  }
+  this->validators_.commit();
+  for (const auto& dbEntry : db.getBatch(this->getNewPrefix("validatorVotes_"))) {
+    this->validatorVotes_.push_back(Utils::fromBigEndian<uint64_t>(dbEntry.value));
+  }
+  this->validatorVotes_.commit();
+  delegationDeltas_.clear();
+  delegationDeltas_.commit();
   doRegister();
 }
 
@@ -419,9 +442,32 @@ void SystemContract::processBlock(std::vector<std::pair<PubKey, uint64_t>>& vali
 
 DBBatch SystemContract::dump() const {
   DBBatch batch = BaseContract::dump();
-
-  // TODO: write state to db
-  //batch.push_back(StrConv::stringToBytes("owner_"), this->owner_.get().asBytes(), this->getDBPrefix());
-
+  batch.push_back(StrConv::stringToBytes("numSlots_"), UintConv::uint64ToBytes(this->numSlots_.get()), this->getDBPrefix());
+  batch.push_back(StrConv::stringToBytes("maxSlots_"), UintConv::uint64ToBytes(this->maxSlots_.get()), this->getDBPrefix());
+  for (auto it = this->targetSlots_.cbegin(); it != this->targetSlots_.cend(); ++it) {
+    batch.push_back(it->first.asBytes(), UintConv::uint64ToBytes(it->second), this->getNewPrefix("targetSlots_"));
+  }
+  for (auto it = this->stakes_.cbegin(); it != this->stakes_.cend(); ++it) {
+    batch.push_back(it->first.asBytes(), UintConv::uint64ToBytes(it->second), this->getNewPrefix("stakes_"));
+  }
+  for (auto it = this->delegations_.cbegin(); it != this->delegations_.cend(); ++it) {
+    for (auto it2 = it->second.cbegin(); it2 != it->second.cend(); ++it2) {
+      const auto& key = it->first; // Address (delegator)
+      Bytes value = it2->first.asBytes(); // PubKey (validator) = 33 bytes
+      Utils::appendBytes(value, UintConv::uint64ToBytes(it2->second)); // uint64_t (votes) = 8 bytes
+      batch.push_back(key, value, this->getNewPrefix("delegations_"));
+    }
+  }
+  for (uint32_t i = 0; i < this->validators_.size(); ++i) {
+    batch.push_back(UintConv::uint32ToBytes(i), this->validators_[i].asBytes(), this->getNewPrefix("validators_"));
+  }
+  for (uint32_t i = 0; i < this->validatorVotes_.size(); ++i) {
+    batch.push_back(UintConv::uint32ToBytes(i), UintConv::uint64ToBytes(this->validatorVotes_[i]), this->getNewPrefix("validatorVotes_"));
+  }
+  if (!delegationDeltas_.empty()) {
+    // delegationDeltas_ *must* be empty both at the start and at the end of block processing.
+    // There is *never* a valid reason to save an inconsistent state snapshot in the middle of block processing.
+    throw DynamicException("System contract is in an inconsistent state during snapshotting (delegationDeltas_ is not empty).");
+  }
   return batch;
 }
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index d16a30f8..d06e63d9 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -93,7 +93,7 @@ class SystemContract : public DynamicContract {
     /// Validator delegations (votes) by users.
     /// Maps voter address to delegations, where each delegation is a validator key
     /// and a 64-bit encoded delegation (vote) amount.
-    SafeUnorderedMap<Address, boost::unordered_flat_map<PubKey, uint64_t>> delegations_;
+    SafeUnorderedMap<Address, boost::unordered_flat_map<PubKey, uint64_t, SafeHash>> delegations_;
 
     /// Sorted list of all validator candidates; the first numSlots_ elements are the elected validators.
     SafeVector<PubKey> validators_;
@@ -179,8 +179,8 @@ class SystemContract : public DynamicContract {
         std::make_tuple("voteSlots", &SystemContract::voteSlots, FunctionTypes::NonPayable, std::vector<std::string>{"validatorPubKey", "slots"})
       );
 
-      // REVIEW: should we add events for validator set updates generated and numSlots changes?
-      //         I guess it wouldn't hurt to have those. Also helps testing (?)
+      // FIXME/TODO: add validator set update events (one per {validator,votes} change)
+      // FIXME/TODO: add numslots update event ({int new_numslots})
       //ContractReflectionInterface::registerContractEvents<SystemContract>(
       //);
     }

From cb7cfefa35aafd05ad3330e7e974e7381789c83a Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Tue, 18 Feb 2025 15:47:49 -0300
Subject: [PATCH 06/15] hook up system contract to consensus + fixes + docs

---
 src/contract/templates/systemcontract.cpp | 25 +++++--
 src/contract/templates/systemcontract.h   | 86 +++++++++++++++-------
 src/core/blockchain.cpp                   | 49 +++++++++++--
 src/core/blockchain.h                     | 21 +++---
 src/core/state.cpp                        | 87 +++++++++++++++++++++--
 src/core/state.h                          | 18 +++--
 src/core/storage.cpp                      |  4 ++
 src/core/storage.h                        | 21 ++++--
 src/utils/db.h                            |  1 +
 src/utils/utils.h                         |  3 +-
 tests/integration/contract/erc20.cpp      |  4 +-
 tests/integration/core/blockchain.cpp     | 23 +++---
 tests/sdktestsuite.cpp                    | 23 +-----
 tests/sdktestsuite.hpp                    |  1 +
 tests/unit/utils/clargs.cpp               | 10 +++
 15 files changed, 276 insertions(+), 100 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index 6c7d54da..d550a58f 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -57,8 +57,9 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
   } else {
     checker -= delta;
   }
-  // the resulting new ranking value for the validator must fit in uint64_t
-  if (checker > std::numeric_limits<uint64_t>::max() || checker < 0) {
+  // the resulting new ranking value for the validator must fit in
+  // **int64_t** (signed) because CometBFT uses int64_t for voting power.
+  if (checker > std::numeric_limits<int64_t>::max() || checker < 0) {
     throw DynamicException("Delegation amount limit exceeded");
   }
   uint64_t targetVotes = checker.convert_to<uint64_t>();
@@ -83,6 +84,8 @@ SystemContract::SystemContract(
   const Address& address, const DB& db
 ) : DynamicContract(address, db)
 {
+  LOGDEBUG("Loading SystemContract...");
+
   this->numSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("numSlots_"), this->getDBPrefix()));
   this->numSlots_.commit();
   this->maxSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("maxSlots_"), this->getDBPrefix()));
@@ -111,6 +114,8 @@ SystemContract::SystemContract(
   delegationDeltas_.clear();
   delegationDeltas_.commit();
   doRegister();
+
+  LOGDEBUG("Loaded SystemContract.");
 }
 
 SystemContract::SystemContract(
@@ -168,7 +173,7 @@ void SystemContract::stake() {
 }
 
 void SystemContract::unstake(const uint256_t& amount) {
-  // Tx native token value transferred is the staking amount, and tx sender is the depositor.
+  // tx sender (caller) is withdrawing amount native token value
   const auto& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
     throw DynamicException("No stake");
@@ -217,7 +222,7 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
       throw DynamicException("Cannot delegate this amount (try a larger amount)");
     }
     --amount64;
-    delegations_[caller][validator] -= 1;
+    --delegations_[caller][validator];
   }
 }
 
@@ -247,14 +252,16 @@ void SystemContract::undelegate(const std::string& validatorPubKey, const uint25
       delegations_.erase(caller);
     }
   }
-  stakes_[caller] += effectiveAmount;
+  if (effectiveAmount > 0) {
+    stakes_[caller] += effectiveAmount;
+  }
   // Loop avoiding collision in delegation amount (which we do not support)
   while (!recordDelegationDelta(validator, amount64, false)) {
     if (amount64 == 1) {
       throw DynamicException("Cannot undelegate this amount (try a larger amount)");
     }
     --amount64;
-    delegations_[caller][validator] += 1;
+    ++delegations_[caller][validator];
   }
 }
 
@@ -279,7 +286,7 @@ void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_
 }
 
 // TODO: call this from incomingBlock()
-void SystemContract::processBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas) {
+void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas) {
   validatorDeltas.clear();
 
   // feed updated validators_ & validatorVotes_ into a sorted validator,votes set
@@ -441,6 +448,8 @@ void SystemContract::processBlock(std::vector<std::pair<PubKey, uint64_t>>& vali
 }
 
 DBBatch SystemContract::dump() const {
+  LOGDEBUG("Saving SystemContract...");
+
   DBBatch batch = BaseContract::dump();
   batch.push_back(StrConv::stringToBytes("numSlots_"), UintConv::uint64ToBytes(this->numSlots_.get()), this->getDBPrefix());
   batch.push_back(StrConv::stringToBytes("maxSlots_"), UintConv::uint64ToBytes(this->maxSlots_.get()), this->getDBPrefix());
@@ -469,5 +478,7 @@ DBBatch SystemContract::dump() const {
     // There is *never* a valid reason to save an inconsistent state snapshot in the middle of block processing.
     throw DynamicException("System contract is in an inconsistent state during snapshotting (delegationDeltas_ is not empty).");
   }
+
+  LOGDEBUG("Saved SystemContract.");
   return batch;
 }
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index d06e63d9..a6ef816c 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -20,23 +20,16 @@ See the LICENSE.txt file in the project root for more information.
  * NOTE: All token amounts are saved internally in uint64_t and multiplied by 1'000'000'000,
  * so any amounts under 0.000000001 are kept by the contract (effectively burned).
  *
- * NOTE: Maximum supported voting power or delegation is ~9.2 billion tokens: it should
- * not be possible to generate that many tokens (total) but we should do our best to
- * protect against this (it's only a problem in the current overflow protection code if
- * intermediary calculations exceed int256_t, so it's never a problem).
+ * NOTE: Maximum supported voting power or delegation is ~9.2 billion tokens (int64_t, since
+ * that's the type used by CometBFT for validator voting power): it should not be possible to
+ * generate that many tokens (total) but we should do our best to protect against this (it's
+ * only a problem in the current overflow protection code if intermediary calculations exceed
+ * int256_t, so it's never a problem).
  *
  * TODO:
  * - Contract class is fully written and compiles.
- * - Register contract class / add to the contract template variant.
- * - Auto-deploy contract on initChain() (should be a fixed contract address)
- *   has to be initChain() since that's the one that gets the genesis file.
- * - Hook Blockchain::incomingBlock() to the system contract.
- *   - normal calls to the contract generate data structure changes
- *   - the newValidators is collected by incomingBlock() at its end, if any
- *     by calling the unregistered contract method directly.
- * - Write cometValidatorUpdates to a DB prefix in the blocks db.
- *   bytes33 pub key + uint64_t value which is the score
  * - Write unit test.
+ *   - Ensure SystemContract cannot be instantiated by incoming transactions.
  * - PR branch.
  */
 class SystemContract : public DynamicContract {
@@ -146,30 +139,74 @@ class SystemContract : public DynamicContract {
       const Address& address, const Address& creator, const uint64_t& chainId
     );
 
-    /// Deposit native tokens
+    /**
+     * An user deposits native tokens into the system contract.
+     * The tx sender (caller) is the depositor, and the tx value is the amount of native tokens deposited.
+     */
     void stake();
 
-    /// Withdraw native tokens
+    /**
+     * An user withdraws native tokens from the system contract.
+     * The tx sender (caller) is requesting the withdrawal.
+     * @param amount Amount of native tokens to withdraw in wei.
+     */
     void unstake(const uint256_t& amount);
 
-    /// Vote for a validator
+    /**
+     * An user votes for a validator.
+     * The tx sender (caller) is the user that is delegating tokens to a validator key.
+     * @param validatorPubKey 33-byte Secp256k1 validador public key to vote, in hex (66 hex char string).
+     * @param amount Amount of staked tokens to delegate to the validator key.
+     */
     void delegate(const std::string& validatorPubKey, const uint256_t& amount);
 
-    /// Unvote a validator
+    /**
+     * An user unvotes from a validator.
+     * The tx sender (caller) is the user that is undelegating tokens from a validator key.
+     * @param validatorPubKey 33-byte Secp256k1 validador public key to unvote, in hex (66 hex char string).
+     * @param amount Amount of staked tokens to undelegate from the validator key.
+     */
     void undelegate(const std::string& validatorPubKey, const uint256_t& amount);
 
-    /// Validator changes a slots vote
-    /// The caller address must match the validator public key
+    /**
+     * An elected validator sets its vote for the number of validator slots.
+     * The tx sender (caller) must be the account for validatorPubKey.
+     * TODO: If we keep an account-to-pubkey reverse mapping, we don't need to take validatorPubKey as a param.
+     * @param validatorPubKey 33-byte Secp256k1 validador public key that is voting for a number of slots (must match caller account).
+     * @param slots Chosen target number of validator slots to vote for.
+     */
     void voteSlots(const std::string& validatorPubKey, const uint64_t& slots);
 
-    /// Apply processing at the end of a block.
-    /// This is an unregistered function called directly by the block processor
-    /// after all block txs are applied to the machine state.
-    void processBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas);
+    /**
+     * Called by the node at the end of block processing to obtain the pending validator set delta.
+     * NOTE: Unregistered function, not callable from other contracts.
+     * @param validatorDeltas Validator set updates, which are validator public keys and their new voting powers (0 to remove).
+     */
+    void finishBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas);
+
+    /**
+     * Get a reference to the ordered validator list.
+     * NOTE: Unregistered function, not callable from other contracts.
+     * @return List of all validator public keys which have delegations (may be more than the number of slots).
+     */
+    SafeVector<PubKey>& getValidators() { return validators_; }
+
+    /**
+     * Get a reference to the ordered validator delegation amounts list.
+     * NOTE: Unregistered function, not callable from other contracts.
+     * @return List of all validator delegations (may be more than the number of slots).
+     */
+    SafeVector<uint64_t>& getValidatorVotes() { return validatorVotes_; };
+
+    /**
+     * Get the current number of validator slots.
+     * NOTE: Unregistered function, not callable from other contracts.
+     * @return Current number of validator slots.
+     */
+    uint64_t getNumSlots() { return numSlots_.get(); }
 
     /// Register the contract.
     static void registerContract() {
-      // TODO: methods
       ContractReflectionInterface::registerContractMethods<SystemContract>(
         std::vector<std::string>{"initialValidators", "initialNumSlots", "maxSlots"},
         std::make_tuple("stake", &SystemContract::stake, FunctionTypes::Payable, std::vector<std::string>{}),
@@ -178,7 +215,6 @@ class SystemContract : public DynamicContract {
         std::make_tuple("undelegate", &SystemContract::undelegate, FunctionTypes::NonPayable, std::vector<std::string>{"validatorPubKey", "amount"}),
         std::make_tuple("voteSlots", &SystemContract::voteSlots, FunctionTypes::NonPayable, std::vector<std::string>{"validatorPubKey", "slots"})
       );
-
       // FIXME/TODO: add validator set update events (one per {validator,votes} change)
       // FIXME/TODO: add numslots update event ({int new_numslots})
       //ContractReflectionInterface::registerContractEvents<SystemContract>(
diff --git a/src/core/blockchain.cpp b/src/core/blockchain.cpp
index 7226b9be..1167f81c 100644
--- a/src/core/blockchain.cpp
+++ b/src/core/blockchain.cpp
@@ -17,6 +17,8 @@ using namespace jsonrpc;
 
 #include "../libs/base64.hpp"
 
+#include "../contract/templates/systemcontract.h"
+
 //#define NODE_DATABASE_DIRECTORY_SUFFIX "/db/"
 
 // ------------------------------------------------------------------
@@ -486,11 +488,11 @@ void Blockchain::initChain(
     throw DynamicException("initChain(): Invalid chain ID: " + std::string(e.what()));
   }
 
-  // For now, the validator set is fixed on genesis and never changes.
-  // TODO: When we get to validator set changes via governance, validators_ will have to be
-  //   updated via incomingBlock(validatorUpdates).
-  // TODO: When loading snapshots, will need to load the validator set at that height as well
-  //   and, maybe, a partial (or total?) history of validator set transitions if that is needed.
+  // Set the initial validator set.
+  // NOTE: The SystemContract has its own idea of what the validator set is (it stores it
+  // in a pair of SafeVector, which is not as useful as the this->validators_ map). However,
+  // both Blockchain and SystemContract should see the same validator set, as the SystemContract
+  // will seed itself from Options::getCometBFT() (which must match &initialValidators).
   setValidators(initialValidators);
 
   // Initialize the machine state on InitChain.
@@ -634,6 +636,40 @@ void Blockchain::incomingBlock(
     //  the obligatory last raw tx, which is the randomness hash and not an actual Eth tx.
     txResults.emplace_back(CometExecTxResult{}); // code==0, gasWanted==0, gasUsed==0
 
+    // Collect validator changes accumulated in the singleton system contract
+    // and return them to CometBFT via the `validatorUpdates` outparam.
+    Bytes validatorDbLog;
+    std::vector<std::pair<PubKey, uint64_t>> validatorDeltas;
+    SystemContract* systemContractPtr = state_.getSystemContract();
+    systemContractPtr->finishBlock(validatorDeltas);
+    for (const auto& validatorDelta : validatorDeltas) {
+      CometValidatorUpdate validatorUpdate;
+      validatorUpdate.publicKey = validatorDelta.first.asBytes();
+      validatorUpdate.power = static_cast<int64_t>(validatorDelta.second);
+      validatorUpdates.push_back(validatorUpdate);
+      Utils::appendBytes(validatorDbLog, validatorUpdate.publicKey);
+      Utils::appendBytes(validatorDbLog, IntConv::int64ToBytes(validatorUpdate.power));
+    }
+    storage_.putValidatorUpdates(block->height, validatorDbLog);
+
+    // Update Blockchain's validator set view to match exactly whatever the SystemContract has
+    SafeVector<PubKey>& scValidators = systemContractPtr->getValidators();
+    SafeVector<uint64_t>& scValidatorVotes = systemContractPtr->getValidatorVotes();
+    uint64_t scNumSlots = systemContractPtr->getNumSlots();
+    if (scValidators.size() != scValidatorVotes.size() || scValidators.size() < scNumSlots) {
+      throw DynamicException("SystemContract has inconsistent validator set data");
+    }
+    std::vector<CometValidatorUpdate> newValidatorSet;
+    for (int i = 0; i < scNumSlots; ++i) {
+      newValidatorSet.emplace_back(
+        CometValidatorUpdate{
+          scValidators[i].asBytes(),
+          static_cast<int64_t>(scValidatorVotes[i])
+        }
+      );
+    }
+    setValidators(newValidatorSet);
+
   } catch (const std::exception& ex) {
     // We need to fail the blockchain node (fatal)
     LOGFATALP_THROW("FATAL: Blockchain::incomingBlock(): " + std::string(ex.what()));
@@ -1170,7 +1206,6 @@ json Blockchain::eth_syncing(const json& request) {
 
 json Blockchain::eth_coinbase(const json& request) {
   forbidParams(request);
-  //return options_.getCoinbase().hex(true);
   Address coinbaseAddress;
   Bytes validatorPubKeyBytes = comet_.getValidatorPubKey();
   // PubKey may be empty in the Comet driver if configuration step hasn't completed yet.
@@ -1216,7 +1251,7 @@ json Blockchain::eth_gasPrice(const json& request) {
 
 json Blockchain::eth_feeHistory(const json& request) {
   // FIXME/TODO: We should probably just have this computed and saved in RAM as we process incoming blocks.
-  // The previous implementation wasn't doing anything (it was just returing a default value, which can be
+  // The previous implementation wasn't doing anything (it was just returning a default value, which can be
   // returned here directly as well); it was just requesting 1,024 blocks from (potentially, or at least now
   // with the new Comet-backed Blockchain::getBlock()) the backing storage, which is expensive and an attack
   // vector on the node, really, and we should just not do that.
diff --git a/src/core/blockchain.h b/src/core/blockchain.h
index 08d46ebc..c5cc3a16 100644
--- a/src/core/blockchain.h
+++ b/src/core/blockchain.h
@@ -19,16 +19,8 @@ See the LICENSE.txt file in the project root for more information.
 
 /**
  * A BDK node.
- * This is the nexus object that brings together multiple blockchain node
- * components by composition. The lifetime of all components and the nexus
- * object are the same.
  * All components must be thread-safe.
  *
- * NOTE: If you need a testing version of a blockchain node, you should derive it
- * from this class, instead of creating another separate class. All components
- * (State, ...) are allowed to expect a mutable Blockchain& in their constructor,
- * so you may need to create a custom one to wrap the component to be tested.
- *
  * NOTE: Each Blockchain instance can have one or more file-backed DBs, which
  * will all be managed by storage_ (Storage class) to implement internal
  * features as needed. However, these should not store:
@@ -81,10 +73,13 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     const std::string instanceId_; ///< Instance ID for logging.
 
+    // NOTE: Comet *must* be destructed (stopped) before State, hence it must be declared after it.
+    // NOTE: HTTPServer should be destructed (stopped) before Comet, hence it should be declared after it.
+
     Options options_; ///< Options singleton.
-    Comet comet_;     ///< CometBFT consensus engine driver.
-    State state_;     ///< Blockchain state.
     Storage storage_; ///< BDK persistent store front-end.
+    State state_;     ///< Blockchain state.
+    Comet comet_;     ///< CometBFT consensus engine driver.
     HTTPServer http_; ///< HTTP server.
 
     // TODO: Encapsulate validator set tracking in a (thread-safe) class (e.g. ValidatorSet).
@@ -160,13 +155,13 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
      */
     json getBlockJson(const FinalizedBlock *block, bool includeTransactions);
 
-  public:
-
     /**
-     * TODO: This should be a private method; it's exposed for unit testing only (should use friend instead).
+     * Update the Blockchain's validator set view.
      */
     void setValidators(const std::vector<CometValidatorUpdate>& newValidatorSet);
 
+  public:
+
     /// A <TxBlock, blockIndex, blockHeight> tuple.
     struct GetTxResultType {
       std::shared_ptr<TxBlock> txBlockPtr; ///< The transaction object.
diff --git a/src/core/state.cpp b/src/core/state.cpp
index 08c2cfa5..a77d8a37 100644
--- a/src/core/state.cpp
+++ b/src/core/state.cpp
@@ -15,6 +15,7 @@ See the LICENSE.txt file in the project root for more information.
 
 #include "../utils/uintconv.h"
 #include "bytes/random.h"
+#include "libs/base64.hpp" // To decode the base64-encoded key strings
 
 #include "blockchain.h"
 
@@ -53,7 +54,7 @@ void State::initChain(
 
   // If the genesis snapshot file param is set, load it.
   if (genesisSnapshot != "") {
-    // Allow V1 snapshots without height_ and timeMicros_
+    // Genesis snapshots can be V1 snapshots without height_ and timeMicros_
     // If these metadata fields are present, they will overwrite the height/time
     //   params supplied to this call and that we set in resetState() above.
     loadSnapshot(genesisSnapshot, true);
@@ -62,6 +63,39 @@ void State::initChain(
   // If set in options, give the chain owner its initial balance
   uint256_t chainOwnerBalance = this->blockchain_.opt().getChainOwnerInitialBalance();
   if (chainOwnerBalance > 0) this->setBalance(this->blockchain_.opt().getChainOwner(), chainOwnerBalance);
+
+  // Initial system contract params (ctor params)
+  std::vector<std::string> initialValidatorPubKeys;
+  const auto& validatorsJson = blockchain_.opt().getCometBFT()[COMET_OPTION_GENESIS_JSON]["validators"];
+  for (const auto& validator : validatorsJson) {
+    std::string validatorPubKeyBase64Str = validator["pub_key"]["value"];
+    Bytes pubBytes = base64::decode_into<Bytes>(validatorPubKeyBase64Str);
+    std::string validatorPubKey = Hex::fromBytes(pubBytes).get();
+    LOGDEBUG("Validator PubKey hex string for SystemContract: " + validatorPubKey);
+    initialValidatorPubKeys.push_back(validatorPubKey);
+  }
+  uint64_t initialNumSlots = initialValidatorPubKeys.size(); // initial numSlots will simply be the number of validators provided at genesis
+  uint64_t maxSlots = 1'000; // Set this to the global maximum numslots for BDK chains
+
+  // FIXME/TODO: Must make the SystemContract template undeployable by user code.
+  //             Only the BDK node's internal code should be able to deploy it.
+
+  LOGDEBUG("Deploying SystemContract...");
+
+  // Create SystemContract account
+  auto& systemContractAcc = *this->accounts_[ProtocolContractAddresses.at("SystemContract")];
+  systemContractAcc.nonce = 1;
+  systemContractAcc.contractType = ContractType::CPP;
+
+  // Create SystemContract contract
+  // We can only construct the SystemContract on initChain() because we need to know the genesis parameters.
+  this->contracts_[ProtocolContractAddresses.at("SystemContract")] =
+    std::make_unique<SystemContract>(
+      initialValidatorPubKeys, initialNumSlots, maxSlots,
+      ProtocolContractAddresses.at("SystemContract"), blockchain_.opt().getChainOwner(), blockchain_.opt().getChainID()
+    );
+
+  LOGDEBUG("Deployed SystemContract.");
 }
 
 std::string State::getLogicalLocation() const { return blockchain_.getLogicalLocation(); }
@@ -100,6 +134,32 @@ void State::contractSanityCheck(const Address& addr, const Account& acc) {
   }
 }
 
+size_t State::getUserContractsSize() {
+  std::shared_lock lock(this->stateMutex_);
+  size_t count = contracts_.size();
+  // Subtract each protocol contract that we find from the user-deployed contracts count.
+  for (const auto& pca : ProtocolContractAddresses) {
+    const Address& protocolContractAddress = pca.second;
+    if (this->contracts_.find(protocolContractAddress) != this->contracts_.end()) {
+      --count;
+    }
+  }
+  return count;
+}
+
+SystemContract* State::getSystemContract() {
+  std::shared_lock lock(this->stateMutex_);
+  if (this->contracts_.find(ProtocolContractAddresses.at("SystemContract")) == this->contracts_.end()) {
+    throw DynamicException("SystemContract not found");
+  }
+  const auto& uniquePtr = this->contracts_[ProtocolContractAddresses.at("SystemContract")];
+  if (uniquePtr.get() == nullptr) {
+    throw DynamicException("SystemContract not found (null)");
+  }
+  // dynamic_cast is not needed here; we know this is a SystemContract
+  return static_cast<SystemContract*>(uniquePtr.get());
+}
+
 void State::resetState(uint64_t height, uint64_t timeMicros) {
   std::unique_lock<std::shared_mutex> lock(stateMutex_);
 
@@ -217,7 +277,7 @@ void State::saveSnapshot(const std::string& where) {
   metaBatch.reset();
 }
 
-void State::loadSnapshot(const std::string& where, bool allowV1Snapshot) {
+void State::loadSnapshot(const std::string& where, bool genesisSnapshot) {
   // NOTE: This method is called in a loop that tries the most recent snapshot first and catches exceptions,
   // then tries the second most recent one and so on, so it's fine to just throw on error.
 
@@ -243,7 +303,7 @@ void State::loadSnapshot(const std::string& where, bool allowV1Snapshot) {
     height_ = UintConv::bytesToUint64(heightBytes);
     LOGXTRACE("Snapshot height: " + std::to_string(height_));
   } else {
-    if (allowV1Snapshot) {
+    if (genesisSnapshot) {
       LOGWARNING("Snapshot has no height field, leaving State height at " + std::to_string(height_));
     } else {
       throw DynamicException("Read corrupt snapshot; missing height_ field.");
@@ -254,7 +314,7 @@ void State::loadSnapshot(const std::string& where, bool allowV1Snapshot) {
     timeMicros_ = UintConv::bytesToUint64(timeMicrosBytes);
     LOGXTRACE("Snapshot timeMicros: " + std::to_string(timeMicros_));
   } else {
-    if (allowV1Snapshot) {
+    if (genesisSnapshot) {
       LOGWARNING("Snapshot has no timeMicros field, leaving State timeMicros at " + std::to_string(timeMicros_));
     } else {
       throw DynamicException("Read corrupt snapshot; missing timeMicros_ field.");
@@ -298,6 +358,25 @@ void State::loadSnapshot(const std::string& where, bool allowV1Snapshot) {
   for (const auto& [addr, acc] : this->accounts_) {
     contractSanityCheck(addr, *acc);
   }
+
+  // SystemContract must be present in non-genesis snapshots.
+  // SystemContract must not be present in genesis snapshots.
+  bool hasSystemContract = (
+    this->contracts_.find(ProtocolContractAddresses.at("SystemContract")) != this->contracts_.end()
+    && this->accounts_.find(ProtocolContractAddresses.at("SystemContract")) != this->accounts_.end()
+  );
+  if (genesisSnapshot && hasSystemContract) {
+    throw DynamicException("Invalid genesis snapshot (has SystemContract).");
+  }
+  if (!genesisSnapshot) {
+    if (!hasSystemContract) {
+      throw DynamicException("Invalid snapshot (missing SystemContract).");
+    }
+    // Make sure it actually loaded
+    if (this->contracts_[ProtocolContractAddresses.at("SystemContract")].get() == nullptr) {
+      throw DynamicException("Invalid snapshot (missing SystemContract: nullptr).");
+    }
+  }
 }
 
 void State::setBalance(const Address& addr, const uint256_t& balance) {
diff --git a/src/core/state.h b/src/core/state.h
index e97cac0a..e89bc2cd 100644
--- a/src/core/state.h
+++ b/src/core/state.h
@@ -61,6 +61,7 @@ using MempoolModelHashIt =
 
 class Blockchain;
 class FinalizedBlock;
+class SystemContract;
 
 #include "typedefs.h"
 
@@ -143,12 +144,16 @@ class State : public Log::LogicalLocationProvider {
     std::string getLogicalLocation() const override; ///< Log helper.
 
     /**
-     * Helper for testing.
+     * Get the number of user-deployed contracts.
+     * @return Number of user-deployed contracts.
      */
-    size_t getContractsSize() {
-      std::shared_lock lock(this->stateMutex_);
-      return contracts_.size();
-    }
+    size_t getUserContractsSize();
+
+    /**
+     * Get the system contract.
+     * @return Pointer to SystemContract, or nullptr if SystemContract not instantiated (no initChain() or loadSnapshot() yet).
+     */
+    SystemContract* getSystemContract();
 
     /**
      * Resets the machine state to its default, bare-minimum state.
@@ -182,8 +187,9 @@ class State : public Log::LogicalLocationProvider {
      * Read the entire consensus machine state held in persistent storage to RAM.
      * May throw on errors.
      * @param where Existing speedb directory name the snapshot will be read from.
+     * @param genesisSnapshot `true` if loading a genesis snapshot, `false` otherwise.
      */
-    void loadSnapshot(const std::string& where, bool allowV1Snapshot = false);
+    void loadSnapshot(const std::string& where, bool genesisSnapshot = false);
 
     /**
      * Get the blockchain block height currently reflected by this machine state.
diff --git a/src/core/storage.cpp b/src/core/storage.cpp
index abf2062b..ee91a327 100644
--- a/src/core/storage.cpp
+++ b/src/core/storage.cpp
@@ -148,3 +148,7 @@ std::vector<Event> Storage::getEvents(uint64_t blockIndex, uint64_t txIndex) con
   return events;
 }
 
+void Storage::putValidatorUpdates(uint64_t height, Bytes validatorUpdates) {
+  blocksDb_.put(UintConv::uint64ToBytes(height), validatorUpdates, DBPrefix::validatorUpdates);
+}
+
diff --git a/src/core/storage.h b/src/core/storage.h
index fdd00abe..4a60145a 100644
--- a/src/core/storage.h
+++ b/src/core/storage.h
@@ -40,6 +40,12 @@ class Storage : public Log::LogicalLocationProvider {
 
     std::string getLogicalLocation() const override; ///< Log helper.
 
+    /**
+     * Get the indexing mode of the storage.
+     * @returns The indexing mode of the storage.
+     */
+    IndexingMode getIndexingMode() const;
+
     /**
      * Stores additional transaction data
      * @param txData The additional transaction data
@@ -91,12 +97,6 @@ class Storage : public Log::LogicalLocationProvider {
      */
     std::vector<Event> getEvents(uint64_t blockIndex, uint64_t txIndex) const;
 
-    /**
-     * Get the indexing mode of the storage.
-     * @returns The indexing mode of the storage.
-     */
-    IndexingMode getIndexingMode() const;
-
     /**
      * Helper function for checking if an event has certain topics.
      * @param event The event to check.
@@ -104,6 +104,15 @@ class Storage : public Log::LogicalLocationProvider {
      * @return `true` if all topics match (or if no topics were provided), `false` otherwise.
      */
     static bool topicsMatch(const Event& event, const std::vector<Hash>& topics);
+
+    /**
+     * Store validator updates.
+     * Validator updates are sequences of 41 bytes, where the first 33 bytes are the
+     * validator's PubKey, and the following 8 bytes are the int64_t vote count.
+     * @param height Block height at which the validator updates have been requested.
+     * @param validatorUpdates Pre-encoded validator updates to store.
+     */
+    void putValidatorUpdates(uint64_t height, Bytes validatorUpdates);
 };
 
 #endif  // STORAGE_H
diff --git a/src/utils/db.h b/src/utils/db.h
index 53c3e662..a0570e61 100644
--- a/src/utils/db.h
+++ b/src/utils/db.h
@@ -26,6 +26,7 @@ namespace DBPrefix {
   const Bytes txToAdditionalData = { 0x00, 0x0A }; ///< "txToAdditionalData" = "000A"
   const Bytes txToCallTrace =      { 0x00, 0x0B }; ///< "txToCallTrace" = "000B"
   const Bytes snapshotMetadata =   { 0x00, 0x0C }; ///< "snapshotMetadata" = "000C"
+  const Bytes validatorUpdates =   { 0x00, 0x0D }; ///< "validatorUpdates" = "000D"
 };
 
 /// Struct for a database connection/endpoint.
diff --git a/src/utils/utils.h b/src/utils/utils.h
index d9de6dcd..ebe436ee 100644
--- a/src/utils/utils.h
+++ b/src/utils/utils.h
@@ -88,7 +88,8 @@ template <typename... Tp> struct printAtExit {
  * Instead, they are deployed in the constructor of State.
  */
 const boost::unordered_flat_map<std::string, Address> ProtocolContractAddresses = {
-  {"ContractManager", Address(Hex::toBytes("0x0001cb47ea6d8b55fe44fdd6b1bdb579efb43e61"))}  // Sha3("ContractManager").substr(0,20)
+  {"ContractManager", Address(Hex::toBytes("0x0001cb47ea6d8b55fe44fdd6b1bdb579efb43e61"))}, // Sha3("ContractManager").substr(0,20)
+  {"SystemContract",  Address(Hex::toBytes("0x0d6e210bae00b550db133853c1e4a96484717512"))}  // Sha3("SystemContract").substr(0,20)
 };
 
 /**
diff --git a/tests/integration/contract/erc20.cpp b/tests/integration/contract/erc20.cpp
index 11ef1553..098dbf0b 100644
--- a/tests/integration/contract/erc20.cpp
+++ b/tests/integration/contract/erc20.cpp
@@ -17,11 +17,11 @@ namespace TERC20 {
   TEST_CASE("ERC20 Class", "[integration][contract][erc20]") {
     SECTION("ERC20 creation") {
       SDKTestSuite sdk = SDKTestSuite::createNewEnvironment("testERC20Creation");
-      REQUIRE(sdk.getState().getContractsSize() == 1);
+      REQUIRE(sdk.getState().getUserContractsSize() == 0);
       Address erc20 = sdk.deployContract<ERC20>(
         std::string("TestToken"), std::string("TST"), uint8_t(18), uint256_t("1000000000000000000")
       );
-      REQUIRE(sdk.getState().getContractsSize() == 2);
+      REQUIRE(sdk.getState().getUserContractsSize() == 1);
       Address owner = sdk.getChainOwnerAccount().address;
       REQUIRE(sdk.callViewFunction(erc20, &ERC20::name) == "TestToken");
       REQUIRE(sdk.callViewFunction(erc20, &ERC20::symbol) == "TST");
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index f9066ebf..7fdec365 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -6,6 +6,7 @@ See the LICENSE.txt file in the project root for more information.
 */
 
 #include "libs/catch2/catch_amalgamated.hpp"
+#include "libs/base64.hpp"
 
 #include "core/blockchain.h"
 
@@ -63,6 +64,20 @@ namespace TBlockchain {
 
       Blockchain blockchain(options, testDumpPath);
 
+      // Here we have to create the CometBFT address that corresponds to the Eth address that we want the coinbase to be set to.
+      // Unfortunately this has to be valid otherwise the coinbase processing step in State::processBlock() will blow up.
+      // Fortunately, we know getOptionsForTest() will use cometTestKeys[0] for our first and only validator.
+      Bytes accValidatorPrivKeyBytes = base64::decode_into<Bytes>(cometTestKeys[0].priv_key);
+      PrivKey accValidatorPrivKey(accValidatorPrivKeyBytes);
+      Bytes accValidatorPubKeyBytes = Secp256k1::toPub(accValidatorPrivKey).asBytes();
+      Bytes accValidatorCometAddress = Comet::getCometAddressFromPubKey(accValidatorPubKeyBytes);
+      TestAccount accValidator(accValidatorPrivKey);
+
+      // Need to emulate initChain() call to force initialization of the validator set.
+      Bytes dummyBytes;
+      std::string chainIdStr = std::to_string(options.getChainID());
+      blockchain.initChain(0, chainIdStr, dummyBytes, 1, {{accValidatorPubKeyBytes, 10}}, dummyBytes);
+
       // For this test, we will not do blockchain start() and stop().
       // Instead, we will just fool Blockchain/State and inject some
       //  TxBlock and FinalizedBlock objects we create here, which is
@@ -79,14 +94,6 @@ namespace TBlockchain {
       // tx A --> AA nonce 3 X token
       // verify all included in block, balance of AA is now 4*X token
 
-      TestAccount accValidator = TestAccount::newRandomAccount();
-
-      // Here we have to create the CometBFT address that corresponds to the Eth address that we want the coinbase to be set to.
-      // Unfortunately this has to be valid otherwise the coinbase processing step in State::processBlock() will blow up.
-      Bytes accValidatorPubKeyBytes = Secp256k1::toPub(accValidator.privKey).asBytes();
-      blockchain.setValidators({{accValidatorPubKeyBytes, 10}}); // second arg is voting power (irrelevant)
-      Bytes accValidatorCometAddress = Comet::getCometAddressFromPubKey(accValidatorPubKeyBytes);
-
       TestAccount accA = TestAccount::newRandomAccount();
       TestAccount accAA = TestAccount::newRandomAccount();
 
diff --git a/tests/sdktestsuite.cpp b/tests/sdktestsuite.cpp
index 75d0546a..f2b099ed 100644
--- a/tests/sdktestsuite.cpp
+++ b/tests/sdktestsuite.cpp
@@ -201,27 +201,6 @@ SDKTestSuite SDKTestSuite::createNewEnvironment(
 ) {
   if (std::filesystem::exists(sdkPath)) std::filesystem::remove_all(sdkPath);
 
-  /*
-    TODO:
-    Default ("genesis") state such as accounts should either:
-    1 - be injected directly into the State.
-    2 - be encoded in a serialized snapshot that is included in the cometBFT
-        genesis data support.
-    Since 1 is easier, we'll do that first during integration since that's
-    mostly for testing. When serialization and deserialization of machine state
-    is added, we can upgrade it to 2.
-    For production we can just hard-code genesis state (i.e. the initial machine
-    state, such as pre-existing accounts and balances, pre-existing deployed
-    contracts at height 0, etc) in the binary -- that is, a hard-coded protocol
-    rule that is implicit.
-    In fact, since the State object and the machine have no support, currently,
-    to be anything other than genesis state (while there's no flat-file
-    serialization and deserialization implemented), then every use case such as
-    tests must inject the starting State on boot since it's always starting
-    from genesis height 0 on node boot (testcases that load or save state from/to
-    the old stateDb are just deleted for now).
-  */
-
   // Create a default options if none is provided.
   std::unique_ptr<Options> options_;
   if (options == nullptr) {
@@ -541,6 +520,8 @@ Options SDKTestSuite::getOptionsForTest(
     validators.push_back(validator);
   }
 
+  // Hack the first validator key
+
   defaultCometBFTOptions["genesis.json"]["validators"] = validators;
 
   defaultCometBFTOptions["genesis.json"]["app_hash"] = appHash;
diff --git a/tests/sdktestsuite.hpp b/tests/sdktestsuite.hpp
index a03f4e09..8ab51429 100644
--- a/tests/sdktestsuite.hpp
+++ b/tests/sdktestsuite.hpp
@@ -217,6 +217,7 @@ class SDKTestSuite : public Blockchain {
      * numNonValidators == 3, then key indices 0..6 are validators, but keys 7..9 are excluded from the
      * validator set (but all 10 nodes are still fully connected to each other via persistent_peers).
      * @param stateDumpTrigger Number of blocks elapsed between Blockchain::saveSnapshot() calls.
+     * @param cometBFTRoundTime CometBFT round time (for each of the 3 rounds).
      * @return Options object set up for testing a Comet instance.
      */
     static Options getOptionsForTest(
diff --git a/tests/unit/utils/clargs.cpp b/tests/unit/utils/clargs.cpp
index d601bf80..97669f6d 100644
--- a/tests/unit/utils/clargs.cpp
+++ b/tests/unit/utils/clargs.cpp
@@ -45,10 +45,19 @@ namespace TClargs {
     }
 
     SECTION("applyProcessOptions") {
+
       // Set all options except log level manually so we can focus on its coverage later
       ProcessOptions opt;
       REQUIRE_FALSE(opt.valid);
       REQUIRE_FALSE(applyProcessOptions(opt));
+
+      /*
+        Tests cannot set process options since process options are process-wide and
+        the global test suite (which is a single process) sets its own process-wide options, which
+        would then get overwritten by this, and thus break the unit test suite.
+        For the process-wide options feature to be testable, some kind of trick is needed, like
+        reversible process options (which is a bad idea; just an example).
+
       opt.valid = true;
       opt.logLevel = "";
       opt.logLineLimit = 1000;
@@ -78,6 +87,7 @@ namespace TClargs {
       REQUIRE(applyProcessOptions(opt2));
       opt2.logLevel = "?"; // Invalid log level
       REQUIRE_FALSE(applyProcessOptions(opt2));
+      */
     }
   }
 }

From 0de2c3c57d91742b10491fd4d58a3ef6b92bcca9 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Tue, 18 Feb 2025 16:38:16 -0300
Subject: [PATCH 07/15] add validator authorization to SystemContract

---
 src/contract/templates/systemcontract.cpp | 53 +++++++++++++++++------
 src/contract/templates/systemcontract.h   |  5 +++
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index d550a58f..f5afaecb 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -11,6 +11,11 @@ See the LICENSE.txt file in the project root for more information.
 
 #include <set>
 
+// Simple protection against instantiating the SystemContract in user code.
+// SystemContract is instantiated by the BDK first, before any user code has a chance to run, which brings the instance count to 1.
+// If any e.g. tx code attempts to create a second instance, it will throw an exception and thus revert.
+std::atomic<int> systemContractInstanceCount = 0;
+
 PubKey SystemContract::pubKeyFromString(const std::string& pubKeyStr) {
   Bytes pubKeyBytes = Hex::toBytes(pubKeyStr);
   if (pubKeyBytes.size() != 33) {
@@ -86,6 +91,11 @@ SystemContract::SystemContract(
 {
   LOGDEBUG("Loading SystemContract...");
 
+  int expected = 0;
+  if (!systemContractInstanceCount.compare_exchange_strong(expected, 1)) {
+    throw DynamicException("Cannot instantiate more than one SystemContract");
+  }
+
   this->numSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("numSlots_"), this->getDBPrefix()));
   this->numSlots_.commit();
   this->maxSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("maxSlots_"), this->getDBPrefix()));
@@ -124,6 +134,11 @@ SystemContract::SystemContract(
   const Address& address, const Address& creator, const uint64_t& chainId
 ) : DynamicContract("SystemContract", address, creator, chainId)
 {
+  int expected = 0;
+  if (!systemContractInstanceCount.compare_exchange_strong(expected, 1)) {
+    throw DynamicException("Cannot instantiate more than one SystemContract");
+  }
+
   std::vector<PubKey> initialValidators;
   for (const auto& pubKeyStr : initialValidatorPubKeys) {
     initialValidators.push_back(pubKeyFromString(pubKeyStr));
@@ -157,6 +172,10 @@ SystemContract::SystemContract(
   doRegister();
 }
 
+SystemContract::~SystemContract() {
+  --systemContractInstanceCount;
+}
+
 void SystemContract::registerContractFunctions() {
   SystemContract::registerContract();
   this->registerMemberFunction("stake", &SystemContract::stake, FunctionTypes::Payable, this);
@@ -174,7 +193,7 @@ void SystemContract::stake() {
 
 void SystemContract::unstake(const uint256_t& amount) {
   // tx sender (caller) is withdrawing amount native token value
-  const auto& caller = this->getCaller();
+  const Address& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
     throw DynamicException("No stake");
   }
@@ -195,7 +214,7 @@ void SystemContract::unstake(const uint256_t& amount) {
 
 void SystemContract::delegate(const std::string& validatorPubKey, const uint256_t& amount) {
   PubKey validator = pubKeyFromString(validatorPubKey);
-  const auto& caller = this->getCaller();
+  const Address& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
     throw DynamicException("No stake");
   }
@@ -210,10 +229,13 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
   if (stakes_[caller] == 0) {
     stakes_.erase(caller);
   }
-  if (delegations_.find(caller) == delegations_.end()) {
-    // It is only possible to delegate to a key if that key has a delegation to itself already.
-    // FIXME/TODO: here, deriving the address of validator must yield the caller address
-    // otherwise throw DynamicException("Unregistered validator")
+  Address vAddr = Secp256k1::toAddress(validator); // Generate validator's eth address from validator pub key
+  if (delegations_.find(vAddr) == delegations_.end() || delegations_[vAddr].find(validator) == delegations_[vAddr].end()) {
+    // It is only possible to delegate to a validator key if that validator key has a delegation to itself already.
+    // Since the validator does not currently have a delegation to itself, this delegation must be from self, that is, caller must be vAddr.
+    if (vAddr != caller) {
+      throw DynamicException("Unregistered validator");
+    }
   }
   delegations_[caller][validator] += amount64;
   // Loop avoiding collision in delegation amount (which we do not support)
@@ -228,7 +250,7 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
 
 void SystemContract::undelegate(const std::string& validatorPubKey, const uint256_t& amount) {
   PubKey validator = pubKeyFromString(validatorPubKey);
-  const auto& caller = this->getCaller();
+  const Address& caller = this->getCaller();
   if (delegations_.find(caller) == delegations_.end()) {
     throw DynamicException("No delegations");
   }
@@ -266,9 +288,17 @@ void SystemContract::undelegate(const std::string& validatorPubKey, const uint25
 }
 
 void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_t& slots) {
+  // Validate numslots choice
+  if (slots < 1 || slots > maxSlots_.get()) {
+    throw DynamicException("Proposed slot count is invalid");
+  }
+  // Authorize caller
   PubKey validator = pubKeyFromString(validatorPubKey);
-  // FIXME/TODO: this->getCaller() address must match validator
-  // i.e. deriving the address from validator (pubkey) must == this->getCaller();
+  Address vAddr = Secp256k1::toAddress(validator); // Generate validator's eth address from validator pub key
+  if (vAddr != this->getCaller()) {
+    throw DynamicException("Caller is not the validator");
+  }
+  // Find elected validator
   bool found = false;
   for (int i = 0; i < numSlots_.get(); ++i) {
     if (validators_[i] == validator) {
@@ -279,13 +309,10 @@ void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_
   if (!found) {
     throw DynamicException("Validator not elected");
   }
-  if (slots < 1 || slots > maxSlots_.get()) {
-    throw DynamicException("Proposed slot count is invalid");
-  }
+  // Save numslots choice
   targetSlots_[validator] = slots;
 }
 
-// TODO: call this from incomingBlock()
 void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas) {
   validatorDeltas.clear();
 
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index a6ef816c..9ce7dd3a 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -139,6 +139,11 @@ class SystemContract : public DynamicContract {
       const Address& address, const Address& creator, const uint64_t& chainId
     );
 
+    /**
+     * Destructor.
+     */
+    virtual ~SystemContract();
+
     /**
      * An user deposits native tokens into the system contract.
      * The tx sender (caller) is the depositor, and the tx value is the amount of native tokens deposited.

From 06e7728cd090942574efaa5580780a7dee79f8a2 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Tue, 18 Feb 2025 21:32:29 -0300
Subject: [PATCH 08/15] WIP SystemContract testcase

---
 src/contract/templates/systemcontract.cpp | 19 -------
 src/contract/templates/systemcontract.h   |  9 +--
 src/core/blockchain.cpp                   | 14 ++++-
 src/core/blockchain.h                     | 12 ++--
 tests/integration/core/blockchain.cpp     | 68 +++++++++++++++++++++++
 tests/sdktestsuite.cpp                    |  8 ++-
 tests/sdktestsuite.hpp                    | 12 ++--
 7 files changed, 103 insertions(+), 39 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index f5afaecb..298782b7 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -11,11 +11,6 @@ See the LICENSE.txt file in the project root for more information.
 
 #include <set>
 
-// Simple protection against instantiating the SystemContract in user code.
-// SystemContract is instantiated by the BDK first, before any user code has a chance to run, which brings the instance count to 1.
-// If any e.g. tx code attempts to create a second instance, it will throw an exception and thus revert.
-std::atomic<int> systemContractInstanceCount = 0;
-
 PubKey SystemContract::pubKeyFromString(const std::string& pubKeyStr) {
   Bytes pubKeyBytes = Hex::toBytes(pubKeyStr);
   if (pubKeyBytes.size() != 33) {
@@ -91,11 +86,6 @@ SystemContract::SystemContract(
 {
   LOGDEBUG("Loading SystemContract...");
 
-  int expected = 0;
-  if (!systemContractInstanceCount.compare_exchange_strong(expected, 1)) {
-    throw DynamicException("Cannot instantiate more than one SystemContract");
-  }
-
   this->numSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("numSlots_"), this->getDBPrefix()));
   this->numSlots_.commit();
   this->maxSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("maxSlots_"), this->getDBPrefix()));
@@ -134,11 +124,6 @@ SystemContract::SystemContract(
   const Address& address, const Address& creator, const uint64_t& chainId
 ) : DynamicContract("SystemContract", address, creator, chainId)
 {
-  int expected = 0;
-  if (!systemContractInstanceCount.compare_exchange_strong(expected, 1)) {
-    throw DynamicException("Cannot instantiate more than one SystemContract");
-  }
-
   std::vector<PubKey> initialValidators;
   for (const auto& pubKeyStr : initialValidatorPubKeys) {
     initialValidators.push_back(pubKeyFromString(pubKeyStr));
@@ -172,10 +157,6 @@ SystemContract::SystemContract(
   doRegister();
 }
 
-SystemContract::~SystemContract() {
-  --systemContractInstanceCount;
-}
-
 void SystemContract::registerContractFunctions() {
   SystemContract::registerContract();
   this->registerMemberFunction("stake", &SystemContract::stake, FunctionTypes::Payable, this);
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index 9ce7dd3a..fe5835f4 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -27,9 +27,11 @@ See the LICENSE.txt file in the project root for more information.
  * int256_t, so it's never a problem).
  *
  * TODO:
+ * - Prevent SystemContract from being instantiated by incoming transactions.
+ * - Actually track pending vs. active validator sets in Blockchain.
+ *   (System contract tracks only the latest elected version and is correct).
  * - Contract class is fully written and compiles.
  * - Write unit test.
- *   - Ensure SystemContract cannot be instantiated by incoming transactions.
  * - PR branch.
  */
 class SystemContract : public DynamicContract {
@@ -139,11 +141,6 @@ class SystemContract : public DynamicContract {
       const Address& address, const Address& creator, const uint64_t& chainId
     );
 
-    /**
-     * Destructor.
-     */
-    virtual ~SystemContract();
-
     /**
      * An user deposits native tokens into the system contract.
      * The tx sender (caller) is the depositor, and the tx value is the amount of native tokens deposited.
diff --git a/src/core/blockchain.cpp b/src/core/blockchain.cpp
index 1167f81c..5b2b7806 100644
--- a/src/core/blockchain.cpp
+++ b/src/core/blockchain.cpp
@@ -109,7 +109,7 @@ Blockchain::Blockchain(const std::string& blockchainPath, std::string instanceId
 {
 }
 
-Blockchain::Blockchain(const Options& options, const std::string& blockchainPath, std::string instanceId)
+Blockchain::Blockchain(const Options& options, std::string instanceId)
   : instanceId_(instanceId),
     options_(options), // copy the given Options object
     comet_(this, instanceId, options_),
@@ -261,6 +261,12 @@ void Blockchain::stop() {
   this->comet_.stop();
 }
 
+Blockchain::~Blockchain() {
+  if (started_) {
+    stop();
+  }
+}
+
 std::shared_ptr<const FinalizedBlock> Blockchain::latest() const {
   return latest_.load();
 }
@@ -271,6 +277,12 @@ uint64_t Blockchain::getLatestHeight() const {
   return latestPtr->getNHeight();
 }
 
+void Blockchain::getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height) {
+  std::unique_lock<std::mutex> lock(validatorMutex_);
+  validatorSet = validators_;
+  height = state_.getHeight();
+}
+
 std::shared_ptr<const FinalizedBlock> Blockchain::getBlock(const Hash& hash) {
   std::shared_ptr<const FinalizedBlock> bp = fbCache_.getByHash(hash);
   if (bp) {
diff --git a/src/core/blockchain.h b/src/core/blockchain.h
index c5cc3a16..480e08a7 100644
--- a/src/core/blockchain.h
+++ b/src/core/blockchain.h
@@ -74,13 +74,12 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
     const std::string instanceId_; ///< Instance ID for logging.
 
     // NOTE: Comet *must* be destructed (stopped) before State, hence it must be declared after it.
-    // NOTE: HTTPServer should be destructed (stopped) before Comet, hence it should be declared after it.
 
     Options options_; ///< Options singleton.
     Storage storage_; ///< BDK persistent store front-end.
     State state_;     ///< Blockchain state.
-    Comet comet_;     ///< CometBFT consensus engine driver.
     HTTPServer http_; ///< HTTP server.
+    Comet comet_;     ///< CometBFT consensus engine driver.
 
     // TODO: Encapsulate validator set tracking in a (thread-safe) class (e.g. ValidatorSet).
     //       We'll be tracking a lot more information about validators when we make the set mutable.
@@ -242,13 +241,12 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     /**
      * Constructor.
-     * @param options Explicit options object to use (overrides any existing options file on blockchainPath).
-     * @param blockchainPath Root filesystem path for this blockchain node reading/writing data.
+     * @param options Explicit options object to use.
      * @param instanceId Runtime object instance identifier for logging (for multi-node unit tests).
      */
-    explicit Blockchain(const Options& options, const std::string& blockchainPath, std::string instanceId = "");
+    explicit Blockchain(const Options& options, std::string instanceId = "");
 
-    ~Blockchain() = default; ///< Default destructor.
+    virtual ~Blockchain(); ///< Destructor.
 
     /**
      * Set the size of the GetTx() cache.
@@ -275,6 +273,8 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     uint64_t getLatestHeight() const; ///< Get the height of the lastest finalized block or 0.
 
+    void getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height); ///< Get the current validator set
+
     /**
      * Given a CometBFT validator address, which is backed by a secp256k1 validator
      * private key (due to how the Comet driver configures CometBFT to use secp256k1
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index 7fdec365..38576b64 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -458,6 +458,74 @@ namespace TBlockchain {
 
       GLOGDEBUG("TEST: done");
     }
+
+    SECTION("BlockchainValidatorSetTest") {
+      const int numNodes = 6;
+      const int numNonValidators = 2;
+      const int numValidators = numNodes - numNonValidators;
+
+      auto ports = SDKTestSuite::generateCometTestPorts(numNodes);
+
+      // Unfortunately, BDK HTTP ports were a late addition to getOptionsForTest()
+      std::vector<int> httpPorts;
+      for (int i = 0; i < numNodes; ++i) { httpPorts.push_back(SDKTestSuite::getTestPort()); }
+
+      std::vector<Options> options;
+      for (int i = 0; i < numNodes; ++i) {
+        options.emplace_back(
+          SDKTestSuite::getOptionsForTest(
+            createTestDumpPath("BlockchainValidatorSetTest_" + std::to_string(i)), false, "",
+            ports[i].p2p, ports[i].rpc, i, numNodes, ports, numNonValidators, 0,
+            "1s", "1s", httpPorts[i]
+          )
+        );
+      }
+
+      std::vector<std::unique_ptr<Blockchain>> blockchains;
+      for (int i = 0; i < numNodes; ++i) {
+        blockchains.emplace_back(std::make_unique<Blockchain>(options[i], std::to_string(i)));
+        // NOTE: Blockchain::start() waits for CometState::RUNNING, so it blocks for a while.
+        //       This is fine (the test still works, nodes eventually manage to dial each other) but
+        //       this could be parallelized so that the test would finish faster.
+        blockchains.back()->start();
+      }
+
+      std::vector<CometValidatorUpdate> validatorSet;
+      uint64_t height = 0;
+
+      // Ensure all nodes think there are 4 validators for a while
+      while (height < 5) {
+        for (int i = 0; i < numNodes; ++i) {
+          blockchains[i]->getValidatorSet(validatorSet, height);
+          REQUIRE(validatorSet.size() == numValidators);
+        }
+        std::this_thread::sleep_for(std::chrono::milliseconds(100));
+      }
+
+      // FIXME: all validator set changes must be tracked as pending, then applied
+      //        this is only affecting the PENDING set
+      //        Blockchain should know precisely which validator set will come into effect when
+      //        For now, test this optimistic version (check directly with the Blockchain class)
+      //        THEN fix it so the Blockchain's view matches CometBFT's for each height.
+
+      // TODO:
+      // Chain owner stakes then delegates 5, 4, 3, 2, 1 tokens for nodes #0 .. #4.
+      // After all transactions went through, validator set should be unchanged.
+
+      // TODO:
+      // Chain owner delegates 10 tokens for node #5
+      // After tx goes through, resulting validator set should be #5, #0, #1, #2 for the 4 slots.
+
+      // TODO:
+      // Validators #5, #0, and #1 vote to change the number of slots: 5, 5, 6
+      // After tx goes through, number of slots should change to 5, making the validator set be: #5, #0, #1, #2, #3
+
+      // TODO:
+      // Validator #1 gets fully undelegated (-4 tokens).
+      // After tx goes through, validator set should be #5, #0, #2, #3, #4
+
+      GLOGDEBUG("TEST: Validator set test finished.");
+    }
   }
 }
 
diff --git a/tests/sdktestsuite.cpp b/tests/sdktestsuite.cpp
index f2b099ed..af73f1eb 100644
--- a/tests/sdktestsuite.cpp
+++ b/tests/sdktestsuite.cpp
@@ -376,7 +376,9 @@ Options SDKTestSuite::getOptionsForTest(
   std::vector<CometTestPorts> ports,
   int numNonValidators,
   int stateDumpTrigger,
-  std::string cometBFTRoundTime
+  std::string cometBFTRoundTime,
+  std::string cometBFTTimeoutCommit,
+  int bdkHttpPort
 ) {
   // Note: all Comet instances are validators.
 
@@ -478,7 +480,7 @@ Options SDKTestSuite::getOptionsForTest(
     {"timeout_prevote_delta", "0s"},
     {"timeout_precommit", cometBFTRoundTime},
     {"timeout_precommit_delta", "0s"},
-    {"timeout_commit", "0s"}
+    {"timeout_commit", cometBFTTimeoutCommit}
   };
 
   // Replace "priv_validator_key.json" with the key at index keyNumber
@@ -533,7 +535,7 @@ Options SDKTestSuite::getOptionsForTest(
     8080,
     Address(Hex::toBytes("0x00dead00665771855a34155f5e7405489df2c3c6")),
     uint256_t(0),
-    9999,
+    bdkHttpPort,
     2000,
     10000,
     stateDumpTrigger,
diff --git a/tests/sdktestsuite.hpp b/tests/sdktestsuite.hpp
index 8ab51429..2ed94cae 100644
--- a/tests/sdktestsuite.hpp
+++ b/tests/sdktestsuite.hpp
@@ -104,7 +104,7 @@ class SDKTestSuite : public Blockchain {
       const Options& options,
       const std::string instanceId = "",
       const std::vector<TestAccount>& accounts = {}
-    ) : Blockchain(options, options.getRootPath(), instanceId), testAccounts_(accounts) {
+    ) : Blockchain(options, instanceId), testAccounts_(accounts) {
       // Existing testcases like SimpleContract don't call start(), so the ctor must start().
       start();
     }
@@ -207,8 +207,8 @@ class SDKTestSuite : public Blockchain {
      * ALL of the peers).
      * @param rootPath Root directory for the testcase.
      * @param appHash Application state hash at genesis.
-     * @param p2pPort The CometBFT P2P local port number to use.
-     * @param rpcPort The CometBFT RPC local port number to use.
+     * @param p2pPort The CometBFT P2P local port number to use (if -1, choose one randomly).
+     * @param rpcPort The CometBFT RPC local port number to use (if -1, choose one randomly).
      * @param keyNumber Index of validator key from the predefined test validator key set.
      * @param numKeys Number of validator keys to include in the genesis spec.
      * @param ports Vector of ports allocated by all peers (unused, if numKeys == 1).
@@ -218,6 +218,8 @@ class SDKTestSuite : public Blockchain {
      * validator set (but all 10 nodes are still fully connected to each other via persistent_peers).
      * @param stateDumpTrigger Number of blocks elapsed between Blockchain::saveSnapshot() calls.
      * @param cometBFTRoundTime CometBFT round time (for each of the 3 rounds).
+     * @param cometBFTTimeoutCommit CometBFT commit timeout.
+     * @param bdkHttpPort The BDK RPC local port number to use (if -1, choose one randomly).
      * @return Options object set up for testing a Comet instance.
      */
     static Options getOptionsForTest(
@@ -229,7 +231,9 @@ class SDKTestSuite : public Blockchain {
       std::vector<CometTestPorts> ports = {},
       int numNonValidators = 0,
       int stateDumpTrigger = 1000,
-      std::string cometBFTRoundTime = "1s"
+      std::string cometBFTRoundTime = "1s",
+      std::string cometBFTTimeoutCommit = "0s",
+      int bdkHttpPort = -1
     );
 
     /**

From 64af7428fcf2285e629cb0599d9a144fe8a2801f Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Wed, 19 Feb 2025 18:28:40 -0300
Subject: [PATCH 09/15] move validator sets from to State + save/load validator
 set schedule

---
 src/core/blockchain.cpp               |  90 ++---------
 src/core/blockchain.h                 |  26 +--
 src/core/state.cpp                    | 218 ++++++++++++++++++++++++--
 src/core/state.h                      |  87 +++++++++-
 src/core/storage.h                    |   2 +
 src/utils/db.h                        |   1 +
 tests/integration/core/blockchain.cpp |  23 +--
 tests/integration/net/httpjsonrpc.cpp |   4 +-
 8 files changed, 309 insertions(+), 142 deletions(-)

diff --git a/src/core/blockchain.cpp b/src/core/blockchain.cpp
index 5b2b7806..1588246c 100644
--- a/src/core/blockchain.cpp
+++ b/src/core/blockchain.cpp
@@ -146,33 +146,6 @@ void Blockchain::putTx(const Hash& tx, const TxCacheValueType& val) {
   txCache_.put(tx, val);
 }
 
-void Blockchain::setValidators(const std::vector<CometValidatorUpdate>& newValidatorSet) {
-  std::unique_lock<std::mutex> lock(validatorMutex_);
-  validators_ = newValidatorSet;
-  validatorAddrs_.clear();
-  for (int i = 0; i < validators_.size(); ++i) {
-    const CometValidatorUpdate& v = validators_[i];
-    Bytes cometAddrBytes = Comet::getCometAddressFromPubKey(v.publicKey);
-    Address cometAddr(cometAddrBytes);
-    validatorAddrs_[cometAddr] = i;
-  }
-}
-
-Address Blockchain::validatorCometAddressToEthAddress(Address validatorCometAddress) {
-  std::unique_lock<std::mutex> lock(validatorMutex_);
-  auto it = validatorAddrs_.find(validatorCometAddress);
-  if (it == validatorAddrs_.end()) {
-    return {};
-  }
-  const uint64_t& validatorIndex = it->second;
-  if (validatorIndex >= validators_.size()) {
-    throw DynamicException("Blockchain::validatorCometAddressToEthAddress() returned an index not in validators_.");
-  }
-  const CometValidatorUpdate& v = validators_[validatorIndex];
-  PubKey pubKey(v.publicKey); // Compressed key (33 bytes)
-  return Secp256k1::toAddress(pubKey); // Generate Eth address from validator pub key
-}
-
 void Blockchain::setGetTxCacheSize(const uint64_t cacheSize) {
   txCache_.resize(cacheSize);
 }
@@ -277,12 +250,6 @@ uint64_t Blockchain::getLatestHeight() const {
   return latestPtr->getNHeight();
 }
 
-void Blockchain::getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height) {
-  std::unique_lock<std::mutex> lock(validatorMutex_);
-  validatorSet = validators_;
-  height = state_.getHeight();
-}
-
 std::shared_ptr<const FinalizedBlock> Blockchain::getBlock(const Hash& hash) {
   std::shared_ptr<const FinalizedBlock> bp = fbCache_.getByHash(hash);
   if (bp) {
@@ -500,13 +467,6 @@ void Blockchain::initChain(
     throw DynamicException("initChain(): Invalid chain ID: " + std::string(e.what()));
   }
 
-  // Set the initial validator set.
-  // NOTE: The SystemContract has its own idea of what the validator set is (it stores it
-  // in a pair of SafeVector, which is not as useful as the this->validators_ map). However,
-  // both Blockchain and SystemContract should see the same validator set, as the SystemContract
-  // will seed itself from Options::getCometBFT() (which must match &initialValidators).
-  setValidators(initialValidators);
-
   // Initialize the machine state on InitChain.
   // NOTE: State height counting is skewed +1 in ABCI Init for some reason.
   //   The genesis.json "initial height" value of 0 is invalid; 1 is the minimum.
@@ -521,7 +481,7 @@ void Blockchain::initChain(
     genesisSnapshot = snapshot0.string();
     LOGINFO("Found genesis snapshot: " + genesisSnapshot);
   }
-  state_.initChain(initialHeight - 1, genesisTime, genesisSnapshot);
+  state_.initChain(initialHeight - 1, genesisTime, initialValidators, genesisSnapshot);
 }
 
 void Blockchain::checkTx(const Bytes& tx, const bool recheck, int64_t& gasWanted, bool& accept) {
@@ -614,13 +574,15 @@ void Blockchain::incomingBlock(
     latest_.store(fbPtr);
     fbCache_.insert(fbPtr);
 
-    // Advance machine state
-    std::vector<bool> succeeded;
-    std::vector<uint64_t> gasUsed;
-
+    // Advance machine state (does ++state_.height_)
+    // Block processing will also generate the validator set updates, since both the
+    //   SystemContract and the validator set schedule (pending and active validator
+    //   sets) are tracked by the State.
     // NOTE: State::processBlock() knows that it has to remove the transactions that
     // are processed into the state from its internal mempool model.
-    state_.processBlock(*fbPtr, succeeded, gasUsed);
+    std::vector<bool> succeeded;
+    std::vector<uint64_t> gasUsed;
+    state_.processBlock(*fbPtr, succeeded, gasUsed, validatorUpdates);
 
     // The last raw tx in the block is not an actual transaction, so it doesn't get an
     // actual tx result generated by processBlock() (which operates on TxBlock objs only).
@@ -648,40 +610,6 @@ void Blockchain::incomingBlock(
     //  the obligatory last raw tx, which is the randomness hash and not an actual Eth tx.
     txResults.emplace_back(CometExecTxResult{}); // code==0, gasWanted==0, gasUsed==0
 
-    // Collect validator changes accumulated in the singleton system contract
-    // and return them to CometBFT via the `validatorUpdates` outparam.
-    Bytes validatorDbLog;
-    std::vector<std::pair<PubKey, uint64_t>> validatorDeltas;
-    SystemContract* systemContractPtr = state_.getSystemContract();
-    systemContractPtr->finishBlock(validatorDeltas);
-    for (const auto& validatorDelta : validatorDeltas) {
-      CometValidatorUpdate validatorUpdate;
-      validatorUpdate.publicKey = validatorDelta.first.asBytes();
-      validatorUpdate.power = static_cast<int64_t>(validatorDelta.second);
-      validatorUpdates.push_back(validatorUpdate);
-      Utils::appendBytes(validatorDbLog, validatorUpdate.publicKey);
-      Utils::appendBytes(validatorDbLog, IntConv::int64ToBytes(validatorUpdate.power));
-    }
-    storage_.putValidatorUpdates(block->height, validatorDbLog);
-
-    // Update Blockchain's validator set view to match exactly whatever the SystemContract has
-    SafeVector<PubKey>& scValidators = systemContractPtr->getValidators();
-    SafeVector<uint64_t>& scValidatorVotes = systemContractPtr->getValidatorVotes();
-    uint64_t scNumSlots = systemContractPtr->getNumSlots();
-    if (scValidators.size() != scValidatorVotes.size() || scValidators.size() < scNumSlots) {
-      throw DynamicException("SystemContract has inconsistent validator set data");
-    }
-    std::vector<CometValidatorUpdate> newValidatorSet;
-    for (int i = 0; i < scNumSlots; ++i) {
-      newValidatorSet.emplace_back(
-        CometValidatorUpdate{
-          scValidators[i].asBytes(),
-          static_cast<int64_t>(scValidatorVotes[i])
-        }
-      );
-    }
-    setValidators(newValidatorSet);
-
   } catch (const std::exception& ex) {
     // We need to fail the blockchain node (fatal)
     LOGFATALP_THROW("FATAL: Blockchain::incomingBlock(): " + std::string(ex.what()));
@@ -1044,7 +972,7 @@ json Blockchain::getBlockJson(const FinalizedBlock *block, bool includeTransacti
   ret["parentHash"] = block->getPrevBlockHash().hex(true);
   ret["sha3Uncles"] = Hash().hex(true); // Uncles do not exist.
 
-  ret["miner"] = validatorCometAddressToEthAddress(block->getProposerAddr()).hex(true);
+  ret["miner"] = state_.validatorCometAddressToEthAddress(block->getProposerAddr()).hex(true);
 
   ret["stateRoot"] = Hash().hex(true); // No State root.
   ret["transactionsRoot"] = block->getTxMerkleRoot().hex(true);
diff --git a/src/core/blockchain.h b/src/core/blockchain.h
index 480e08a7..8fbcb13e 100644
--- a/src/core/blockchain.h
+++ b/src/core/blockchain.h
@@ -81,12 +81,6 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
     HTTPServer http_; ///< HTTP server.
     Comet comet_;     ///< CometBFT consensus engine driver.
 
-    // TODO: Encapsulate validator set tracking in a (thread-safe) class (e.g. ValidatorSet).
-    //       We'll be tracking a lot more information about validators when we make the set mutable.
-    std::vector<CometValidatorUpdate> validators_; ///< Up-to-date CometBFT validator set.
-    std::unordered_map<Address, uint64_t, SafeHash> validatorAddrs_; /// Up-to-date map of CometBFT validator address to index in validators_.
-    std::mutex validatorMutex_; ///< Protects validators_ and validatorAddrs_
-
     // FIXME/TODO: Need to query for the last block and fill this in on boot.
     //             (That initial block query will also feed the fbCache_).
     std::atomic<std::shared_ptr<const FinalizedBlock>> latest_; ///< Pointer to the latest block in the blockchain.
@@ -154,11 +148,6 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
      */
     json getBlockJson(const FinalizedBlock *block, bool includeTransactions);
 
-    /**
-     * Update the Blockchain's validator set view.
-     */
-    void setValidators(const std::vector<CometValidatorUpdate>& newValidatorSet);
-
   public:
 
     /// A <TxBlock, blockIndex, blockHeight> tuple.
@@ -271,20 +260,7 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     std::shared_ptr<const FinalizedBlock> latest() const; ///< Get latest finalized block.
 
-    uint64_t getLatestHeight() const; ///< Get the height of the lastest finalized block or 0.
-
-    void getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height); ///< Get the current validator set
-
-    /**
-     * Given a CometBFT validator address, which is backed by a secp256k1 validator
-     * private key (due to how the Comet driver configures CometBFT to use secp256k1
-     * keys), look up the current validator list, find the validator private key given
-     * the CometBFT validator address, then generate an Eth validator address from
-     * that found private key.
-     * @param validatorCometAddress The CometBFT address of one of the active validators.
-     * @return The translation of the CometBFT address into the corresponding Eth address.
-     */
-    Address validatorCometAddressToEthAddress(Address validatorCometAddress);
+    uint64_t getLatestHeight() const; ///< Get the height of the lastest finalized block, or 0 if no blocks (genesis state).
 
     /**
      * Get a block from the chain using a given hash.
diff --git a/src/core/state.cpp b/src/core/state.cpp
index a77d8a37..16081c23 100644
--- a/src/core/state.cpp
+++ b/src/core/state.cpp
@@ -8,6 +8,7 @@ See the LICENSE.txt file in the project root for more information.
 #include <evmone/evmone.h>
 
 #include "state.h"
+#include "blockchain.h"
 
 #include "../contract/contracthost.h" // contractmanager.h
 #include "../contract/contractfactory.h" // ContractFactory
@@ -17,8 +18,6 @@ See the LICENSE.txt file in the project root for more information.
 #include "bytes/random.h"
 #include "libs/base64.hpp" // To decode the base64-encoded key strings
 
-#include "blockchain.h"
-
 // Only need to register contract templates once
 std::once_flag State::stateRegisterContractsFlag;
 void State::registerContracts() { ContractFactory::registerContracts<ContractTypes>(); }
@@ -44,7 +43,9 @@ State::State(Blockchain& blockchain) : blockchain_(blockchain), vm_(evmc_create_
 State::~State() { evmc_destroy(this->vm_); }
 
 void State::initChain(
-  uint64_t initialHeight, uint64_t initialTimeEpochSeconds, std::string genesisSnapshot
+  uint64_t initialHeight, uint64_t initialTimeEpochSeconds,
+  const std::vector<CometValidatorUpdate>& initialValidators,
+  std::string genesisSnapshot
 ) {
   LOGDEBUG("State::initChain(): Height (BDK, -1) = " + std::to_string(initialHeight));
 
@@ -66,14 +67,22 @@ void State::initChain(
 
   // Initial system contract params (ctor params)
   std::vector<std::string> initialValidatorPubKeys;
-  const auto& validatorsJson = blockchain_.opt().getCometBFT()[COMET_OPTION_GENESIS_JSON]["validators"];
-  for (const auto& validator : validatorsJson) {
-    std::string validatorPubKeyBase64Str = validator["pub_key"]["value"];
-    Bytes pubBytes = base64::decode_into<Bytes>(validatorPubKeyBase64Str);
-    std::string validatorPubKey = Hex::fromBytes(pubBytes).get();
-    LOGDEBUG("Validator PubKey hex string for SystemContract: " + validatorPubKey);
-    initialValidatorPubKeys.push_back(validatorPubKey);
+  // // Don't need to fetch them from the Options as we get the exact same info via ABCI InitChain params
+  // const auto& validatorsJson = blockchain_.opt().getCometBFT()[COMET_OPTION_GENESIS_JSON]["validators"];
+  // for (const auto& validator : validatorsJson) {
+  //   std::string validatorPubKeyBase64Str = validator["pub_key"]["value"];
+  //   Bytes pubBytes = base64::decode_into<Bytes>(validatorPubKeyBase64Str);
+  //   std::string validatorPubKey = Hex::fromBytes(pubBytes).get();
+  //   LOGDEBUG("Validator PubKey hex string for SystemContract: " + validatorPubKey);
+  //   initialValidatorPubKeys.push_back(validatorPubKey);
+  // }
+  for (const auto& validator : initialValidators) {
+    // SystemContract takes public keys as hex strings instead of PubKey, unfortunately.
+    std::string validatorPubKeyStr = Hex::fromBytes(validator.publicKey).get();
+    initialValidatorPubKeys.push_back(validatorPubKeyStr);
+    LOGDEBUG("Validator PubKey hex string for SystemContract: " + validatorPubKeyStr);
   }
+
   uint64_t initialNumSlots = initialValidatorPubKeys.size(); // initial numSlots will simply be the number of validators provided at genesis
   uint64_t maxSlots = 1'000; // Set this to the global maximum numslots for BDK chains
 
@@ -96,6 +105,55 @@ void State::initChain(
     );
 
   LOGDEBUG("Deployed SystemContract.");
+
+  // Set the initial validator set.
+  // NOTE: The SystemContract has its own idea of what the validator set is (it stores it
+  // in a pair of SafeVector, which is not as useful as the this->validators_ map). However,
+  // both Blockchain and SystemContract should see the same validator set, as the SystemContract
+  // will seed itself from Options::getCometBFT() (which must match &initialValidators).
+  setValidators(initialValidators);
+}
+
+void State::setValidators(const std::vector<CometValidatorUpdate>& newValidatorSet) {
+  // NOTE: stateMutex_ should already be locked by caller, if necessary
+
+  // Normally, a validator set that is chosen in block H becomes active in H+2.
+  // However, if we have no validator sets, that means we are getting the genesis validator set
+  //   which is automatically active at the current State height.
+  uint64_t activationHeight = height_;
+  if (currentValidatorSet_ >= 0) {
+    // The set we are pushing at the front will be the currentValidatorSet two blocks from now.
+    activationHeight = height_ + 2;
+    // Since we pushed a new element, the current set is deeper in the deque now.
+    ++currentValidatorSet_;
+  } else {
+    // Point to the genesis set we are creating.
+    currentValidatorSet_ = 0;
+  }
+  validatorSets_.emplace_front(
+    ValidatorSet(
+      activationHeight,
+      newValidatorSet
+    )
+  );
+}
+
+Address State::validatorCometAddressToEthAddress(Address validatorCometAddress) {
+  //std::shared_lock<std::shared_mutex> lock(stateMutex_);
+  if (currentValidatorSet_ < 0) {
+    throw DynamicException("No validator set: " + std::to_string(validatorSets_.size()));
+  }
+  return validatorSets_[currentValidatorSet_].validatorCometAddressToEthAddress(validatorCometAddress);
+}
+
+// Returns the currently active validator set and the height in which it became active
+void State::getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height) {
+  //std::shared_lock<std::shared_mutex> lock(stateMutex_);
+  if (currentValidatorSet_ < 0) {
+    throw DynamicException("No validator set: " + std::to_string(validatorSets_.size()));
+  }
+  validatorSet = validatorSets_[currentValidatorSet_].getValidators();
+  height = validatorSets_[currentValidatorSet_].getHeight();
 }
 
 std::string State::getLogicalLocation() const { return blockchain_.getLogicalLocation(); }
@@ -148,7 +206,11 @@ size_t State::getUserContractsSize() {
 }
 
 SystemContract* State::getSystemContract() {
-  std::shared_lock lock(this->stateMutex_);
+  std::shared_lock<std::shared_mutex> lock(stateMutex_);
+  return getSystemContractInternal();
+}
+
+SystemContract* State::getSystemContractInternal() {
   if (this->contracts_.find(ProtocolContractAddresses.at("SystemContract")) == this->contracts_.end()) {
     throw DynamicException("SystemContract not found");
   }
@@ -172,6 +234,10 @@ void State::resetState(uint64_t height, uint64_t timeMicros) {
   this->vmStorage_.clear();
   this->accounts_.clear();
 
+  // Wipe the validator set schedule
+  validatorSets_.clear();
+  currentValidatorSet_ = -1;
+
   // Create ContractManager account
   auto& contractManagerAcc = *this->accounts_[ProtocolContractAddresses.at("ContractManager")];
   contractManagerAcc.nonce = 1;
@@ -221,6 +287,30 @@ void State::saveSnapshot(const std::string& where) {
   out.putBatch(*metaBatch);
   metaBatch.reset();
 
+  // Write the validator set schedule (all validator sets that State is keeping track of).
+  // We need this when restoring a machine state because the SystemContract only keeps
+  // the latest elected validator set (in other words, State does not delegate this
+  // responsibility to the governance contract; the governance contract can be changed
+  // to handle chain governance differently with a reduced risk of affecting the machine
+  // plus consensus engine environment in an unintended way).
+  std::unique_ptr<DBBatch> validatorBatch = std::make_unique<DBBatch>();
+  for (int i = 0; i < validatorSets_.size(); ++i) {
+    const std::vector<CometValidatorUpdate>& validators = validatorSets_[i].getValidators();
+    const uint64_t& height = validatorSets_[i].getHeight();
+    Bytes serializedValidatorSet;
+    for (int j = 0; j < validators.size(); ++j) {
+      Utils::appendBytes(serializedValidatorSet, validators[i].publicKey);
+      Utils::appendBytes(serializedValidatorSet, IntConv::int64ToBytes(validators[i].power));
+    }
+    validatorBatch->push_back(
+      UintConv::uint64ToBytes(height),
+      serializedValidatorSet,
+      DBPrefix::validatorSets
+    );
+  }
+  out.putBatch(*validatorBatch);
+  validatorBatch.reset();
+
   size_t count;
 
   // Write accounts_ in batches of 1,000; uses a bit more memory but should be faster
@@ -321,6 +411,43 @@ void State::loadSnapshot(const std::string& where, bool genesisSnapshot) {
     }
   }
 
+  // Load the validator set schedule
+  uint64_t vsetCount = 0;
+  auto vsetsFromDB = in.getBatch(DBPrefix::validatorSets);
+  currentValidatorSet_ = vsetsFromDB.size();
+  for (const auto& dbEntry : vsetsFromDB) {
+    uint64_t height = UintConv::bytesToUint64(dbEntry.key);
+    const Bytes& validatorSetBytes = dbEntry.value;
+    // A serialized Secp256k1 PubKey is 33 bytes + 8 bytes int64_t voting power = 41
+    if (validatorSetBytes.size() % 41 != 0) {
+      throw DynamicException("Read corrupt snapshot; a serialized validator set is truncated.");
+    }
+    int validatorCount = validatorSetBytes.size() / 41;
+    std::vector<CometValidatorUpdate> validators;
+    for (int i = 0; i < validatorCount; ++i) {
+      int offset = 41 * i;
+      Bytes validator(validatorSetBytes.begin() + offset, validatorSetBytes.begin() + offset + 33);
+      Bytes votesBytes(validatorSetBytes.begin() + offset + 33, validatorSetBytes.begin() + offset + 41);
+      int64_t votes = IntConv::bytesToInt64(votesBytes);
+      validators.push_back(
+        CometValidatorUpdate{
+          validator,
+          votes
+        }
+      );
+    }
+    validatorSets_.push_front(ValidatorSet(height, validators)); // DB is a sorted container, so push oldest height first
+    if (height <= height_) {
+      // This validator set is not yet one that is in the future, so it is the next candidate.
+      --currentValidatorSet_;
+    }
+    ++vsetCount;
+  }
+  if (currentValidatorSet_ == vsetsFromDB.size()) {
+    throw DynamicException("Read corrupt snapshot: no validator set is current");
+  }
+  LOGXTRACE("Validator sets size: " + std::to_string(vsetCount));
+
   // Load accounts_
   uint64_t accCount = 0;
   auto accountsFromDB = in.getBatch(DBPrefix::nativeAccounts);
@@ -366,6 +493,8 @@ void State::loadSnapshot(const std::string& where, bool genesisSnapshot) {
     && this->accounts_.find(ProtocolContractAddresses.at("SystemContract")) != this->accounts_.end()
   );
   if (genesisSnapshot && hasSystemContract) {
+    // REVIEW: We could also just explicitly ignore (skip) the SystemContract when loading a genesis snapshot,
+    //         instead of considering the whole snapshot invalid.
     throw DynamicException("Invalid genesis snapshot (has SystemContract).");
   }
   if (!genesisSnapshot) {
@@ -799,7 +928,10 @@ bool State::validateTransactionInternal(const TxBlock& tx, bool affectsMempool,
   return true;
 }
 
-void State::processBlock(const FinalizedBlock& block, std::vector<bool>& succeeded, std::vector<uint64_t>& gasUsed) {
+void State::processBlock(
+  const FinalizedBlock& block, std::vector<bool>& succeeded, std::vector<uint64_t>& gasUsed,
+  std::vector<CometValidatorUpdate>& validatorUpdates
+) {
   std::unique_lock lock(this->stateMutex_);
 
   // NOTE: Block should already have been validated by the caller.
@@ -826,7 +958,7 @@ void State::processBlock(const FinalizedBlock& block, std::vector<bool>& succeed
   // Address derivation schemes (from the same Secp256k1 public key) differ between CometBFT and Eth.
   // So we need to map CometBFT Address to CometValidatorUpdate (a validator public key)
   //   and then use the validator public key to compute the correct Eth Address.
-  Address proposerEthAddr = blockchain_.validatorCometAddressToEthAddress(block.getProposerAddr());
+  Address proposerEthAddr = blockchain_.state().validatorCometAddressToEthAddress(block.getProposerAddr());
   ContractGlobals::coinbase_ = proposerEthAddr;
   // LOGTRACE("Coinbase set to: " + proposerEthAddr.hex().get() + " (CometBFT Addr: " + block.getProposerAddr().hex().get() + ")");
 
@@ -844,6 +976,66 @@ void State::processBlock(const FinalizedBlock& block, std::vector<bool>& succeed
 
   // Update the state height after processing
   height_ = block.getNHeight();
+
+  // Check if SystemContract has validator set updates for this block
+  std::vector<std::pair<PubKey, uint64_t>> validatorDeltas;
+  SystemContract* systemContractPtr = getSystemContractInternal();
+  systemContractPtr->finishBlock(validatorDeltas); // Collect any validator changes accumulated in the singleton system contract...
+
+  if (!validatorDeltas.empty()) {
+    // Apply validator set deltas
+    Bytes validatorDbLog;
+    for (const auto& validatorDelta : validatorDeltas) {
+      CometValidatorUpdate validatorUpdate;
+      validatorUpdate.publicKey = validatorDelta.first.asBytes();
+      validatorUpdate.power = static_cast<int64_t>(validatorDelta.second);
+      validatorUpdates.push_back(validatorUpdate); // ...and return them to CometBFT via the `validatorUpdates` outparam...
+      Utils::appendBytes(validatorDbLog, validatorUpdate.publicKey);
+      Utils::appendBytes(validatorDbLog, IntConv::int64ToBytes(validatorUpdate.power));
+    }
+    blockchain_.storage().putValidatorUpdates(height_, validatorDbLog); // ...store all validator set changes into the local DB...
+
+    // Update Blockchain's validator set view to match exactly whatever the SystemContract has
+    // NOTE: Blockchain::setValidators() gets the given newValidatorSet below, and it understands that it is
+    //       receiving that new validator set at the current state_.height_; and it also understands that
+    //       the newValidatorSet will only become active in state_.height_ + 2. So it will actually queue this
+    //       newValidatorSet for future use.
+    //       This contrasts with the SystemContract's tracking of validator sets: it is only concerned about which
+    //       validators are currently *elected*, as the SystemContract is a governance mechanism.
+    SafeVector<PubKey>& scValidators = systemContractPtr->getValidators();
+    SafeVector<uint64_t>& scValidatorVotes = systemContractPtr->getValidatorVotes();
+    uint64_t scNumSlots = systemContractPtr->getNumSlots();
+    if (scValidators.size() != scValidatorVotes.size() || scValidators.size() < scNumSlots) {
+      throw DynamicException("SystemContract has inconsistent validator set data");
+    }
+    std::vector<CometValidatorUpdate> newValidatorSet;
+    for (int i = 0; i < scNumSlots; ++i) {
+      newValidatorSet.emplace_back(
+        CometValidatorUpdate{
+          scValidators[i].asBytes(),
+          static_cast<int64_t>(scValidatorVotes[i]) // nobody is getting > 9.2 billion native token votes
+        }
+      );
+    }
+    setValidators(newValidatorSet);
+  }
+
+  // Since we have advanced the simulation height, we may need to prune validatorSets_
+  // Reverse search for the first validator set that is pending, and for each iteration,
+  //   add 1 to the count of old validatorSets_ entires to prune.
+  // Example: if no pending validator sets exist (all are active), will prune all but the front.
+  // Example: [pending pending active active active] should prune the last 2.
+  int pruneCount = -1; // Start at -1 so the last active is preserved.
+  for (auto it = validatorSets_.rbegin(); it != validatorSets_.rend(); ++it) {
+    if (it->getHeight() < height_) {
+      break; // found first pending validator set, so stop
+    }
+    ++pruneCount;
+  }
+  while (pruneCount > 0) {
+    validatorSets_.pop_back();
+    --pruneCount;
+  }
 }
 
 void State::processTransaction(
diff --git a/src/core/state.h b/src/core/state.h
index e89bc2cd..d85c6394 100644
--- a/src/core/state.h
+++ b/src/core/state.h
@@ -16,10 +16,8 @@ See the LICENSE.txt file in the project root for more information.
 #include "../utils/logger.h"
 #include "../utils/safehash.h"
 
-#if defined(BUILD_TESTS) && defined(BUILD_BENCHMARK_TESTS)
-// Forward declaration. Used only for benchmarking purposes.
-class SDKTestSuite;
-#endif
+#include "typedefs.h"
+#include "comet.h"
 
 /// Mempool model to help validate multiple txs with same from account and various nonce values.
 using MempoolModel =
@@ -59,11 +57,47 @@ using MempoolModelHashIt =
     >
   , SafeHash>::iterator;
 
+#if defined(BUILD_TESTS) && defined(BUILD_BENCHMARK_TESTS)
+// Forward declaration. Used only for benchmarking purposes.
+class SDKTestSuite;
+#endif
 class Blockchain;
 class FinalizedBlock;
 class SystemContract;
 
-#include "typedefs.h"
+/// A CometBFT validator set that becomes active at a given height
+class ValidatorSet {
+  private:
+    uint64_t height_; ///< Height of the first block that this validator set actually votes on (is active, in effect).
+    std::vector<CometValidatorUpdate> validators_; ///< A CometBFT validator set.
+    std::unordered_map<Address, uint64_t, SafeHash> validatorAddrs_; /// Map of CometBFT validator address to index in validators_.
+  public:
+    uint64_t& getHeight() { return height_; }
+    const std::vector<CometValidatorUpdate>& getValidators() { return validators_; }
+    Address validatorCometAddressToEthAddress(const Address& validatorCometAddress) {
+      auto it = validatorAddrs_.find(validatorCometAddress);
+      if (it == validatorAddrs_.end()) {
+        return {};
+      }
+      const uint64_t& validatorIndex = it->second;
+      if (validatorIndex >= validators_.size()) {
+        throw DynamicException("Blockchain::validatorCometAddressToEthAddress() returned an index not in validators_.");
+      }
+      const CometValidatorUpdate& v = validators_[validatorIndex];
+      PubKey pubKey(v.publicKey); // Compressed key (33 bytes)
+      return Secp256k1::toAddress(pubKey); // Generate Eth address from validator pub key
+    }
+    ValidatorSet(const uint64_t& height, const std::vector<CometValidatorUpdate>& validators)
+      : height_(height), validators_(validators)
+    {
+      for (int i = 0; i < validators_.size(); ++i) {
+        const CometValidatorUpdate& v = validators_[i];
+        Bytes cometAddrBytes = Comet::getCometAddressFromPubKey(v.publicKey);
+        Address cometAddr(cometAddrBytes);
+        validatorAddrs_[cometAddr] = i;
+      }
+    }
+};
 
 /// Abstraction of the blockchain's current state at the current block.
 class State : public Log::LogicalLocationProvider {
@@ -83,6 +117,16 @@ class State : public Log::LogicalLocationProvider {
     ContractsContainerType contracts_; ///< Map with information about blockchain contracts (Address -> Contract).
     boost::unordered_flat_map<StorageKey, Hash, SafeHash, SafeCompare> vmStorage_; ///< Map with the storage of the EVM.
     boost::unordered_flat_map<Address, NonNullUniquePtr<Account>, SafeHash, SafeCompare> accounts_; ///< Map with information about blockchain accounts (Address -> Account).
+    int currentValidatorSet_ = -1; ///< Index in validatorSets_ of the currently active validator set, -1 means none.
+    std::deque<ValidatorSet> validatorSets_; ///< More recent set in front, oldest in back.
+
+    /**
+     * A new validator set is elected in the governance contract, so update the State with it.
+     * @param newValidatorSet Validator set that will become active in current height + 2 or, if we are in
+     * genesis state, the genesis validator set that becomes the first validator set (instantly active at
+     * starting height).
+     */
+    void setValidators(const std::vector<CometValidatorUpdate>& newValidatorSet);
 
     /**
      * Helper for static contract registration step.
@@ -129,6 +173,12 @@ class State : public Log::LogicalLocationProvider {
      */
     void contractSanityCheck(const Address& addr, const Account& acc);
 
+    /**
+     * Get the system contract. Does not acquire the stateMutex_.
+     * @return Pointer to SystemContract, or nullptr if SystemContract not instantiated (no initChain() or loadSnapshot() yet).
+     */
+    SystemContract* getSystemContractInternal();
+
     #if defined(BUILD_TESTS) && defined(BUILD_BENCHMARK_TESTS)
     // Process a transaction directly without having to put it in a block.
     // FOR TESTING PURPOSES ONLY. DO NOT COMPILE FOR PRODUCTION
@@ -143,6 +193,25 @@ class State : public Log::LogicalLocationProvider {
 
     std::string getLogicalLocation() const override; ///< Log helper.
 
+    /**
+     * Get the validator set that is currently in effect.
+     * @param validatorSet Outparam with the validator set in effect.
+     * @param height Outparam with the block height in which this validator set took effect
+     * (past or present height) or will take effect (future height).
+     */
+    void getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height);
+
+    /**
+     * Given a CometBFT validator address, which is backed by a secp256k1 validator
+     * private key (due to how the Comet driver configures CometBFT to use secp256k1
+     * keys), look up the current validator list, find the validator private key given
+     * the CometBFT validator address, then generate an Eth validator address from
+     * that found private key.
+     * @param validatorCometAddress The CometBFT address of one of the active validators.
+     * @return The translation of the CometBFT address into the corresponding Eth address.
+     */
+    Address validatorCometAddressToEthAddress(Address validatorCometAddress);
+
     /**
      * Get the number of user-deployed contracts.
      * @return Number of user-deployed contracts.
@@ -169,10 +238,12 @@ class State : public Log::LogicalLocationProvider {
      * Genesis state is known only after the ABCI InitChain call.
      * @param initialHeight Genesis height (0 is the default start-from-scratch, no-blocks value).
      * @param initialTimeEpochSeconds Genesis timestamp in seconds since epoch.
+     * @param initialValidators Validator set at genesis.
      * @param genesisSnapshot Optional snapshot directory location with genesis state to load ("" if none).
      */
     void initChain(
       uint64_t initialHeight, uint64_t initialTimeEpochSeconds,
+      const std::vector<CometValidatorUpdate>& initialValidators,
       std::string genesisSnapshot = ""
     );
 
@@ -236,9 +307,13 @@ class State : public Log::LogicalLocationProvider {
      * @param block The block to process.
      * @param succeeded Empty outparam vector of tx execution success status.
      * @param gasUsed Empty outparam vector of tx execution gas used.
+     * @param validatorUpdates Empty outparam filled in with (optional) validator updates for CometBFT.
      * @throw DynamicException on any error.
      */
-    void processBlock(const FinalizedBlock& block, std::vector<bool>& succeeded, std::vector<uint64_t>& gasUsed);
+    void processBlock(
+      const FinalizedBlock& block, std::vector<bool>& succeeded, std::vector<uint64_t>& gasUsed,
+      std::vector<CometValidatorUpdate>& validatorUpdates
+    );
 
     /**
      * Verify if a transaction can be accepted within the current state and mempool model.
diff --git a/src/core/storage.h b/src/core/storage.h
index 4a60145a..a27a8686 100644
--- a/src/core/storage.h
+++ b/src/core/storage.h
@@ -107,6 +107,8 @@ class Storage : public Log::LogicalLocationProvider {
 
     /**
      * Store validator updates.
+     * By saving all validator updates, we can reconstruct the full history of validator sets if required.
+     * This full validator update history is not saved in snapshots.
      * Validator updates are sequences of 41 bytes, where the first 33 bytes are the
      * validator's PubKey, and the following 8 bytes are the int64_t vote count.
      * @param height Block height at which the validator updates have been requested.
diff --git a/src/utils/db.h b/src/utils/db.h
index a0570e61..c0520c68 100644
--- a/src/utils/db.h
+++ b/src/utils/db.h
@@ -27,6 +27,7 @@ namespace DBPrefix {
   const Bytes txToCallTrace =      { 0x00, 0x0B }; ///< "txToCallTrace" = "000B"
   const Bytes snapshotMetadata =   { 0x00, 0x0C }; ///< "snapshotMetadata" = "000C"
   const Bytes validatorUpdates =   { 0x00, 0x0D }; ///< "validatorUpdates" = "000D"
+  const Bytes validatorSets =      { 0x00, 0x0E }; ///< "validatorSets" = "000E"
 };
 
 /// Struct for a database connection/endpoint.
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index 38576b64..5d86c744 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -180,8 +180,9 @@ namespace TBlockchain {
       // Create a BDK FinalizedBlock from the fake ABCI block and send it to the machine state
       std::vector<bool> succeeded;
       std::vector<uint64_t> gasUsed;
+      std::vector<CometValidatorUpdate> validatorUpdates;
       FinalizedBlock finBlock1 = FinalizedBlock::fromCometBlock(cometBlock);
-      blockchain.state().processBlock(finBlock1, succeeded, gasUsed);
+      blockchain.state().processBlock(finBlock1, succeeded, gasUsed, validatorUpdates);
 
       REQUIRE(succeeded.size() == 4);
       REQUIRE(succeeded[0] == true);
@@ -337,7 +338,7 @@ namespace TBlockchain {
       gasUsed.clear();
       cometBlock.txs.push_back(Utils::randBytes(32)); // append a randomHash non-tx tx (required by our protocol / FinalizedBlock::fromCometBlock())
       FinalizedBlock finBlock2 = FinalizedBlock::fromCometBlock(cometBlock);
-      blockchain.state().processBlock(finBlock2, succeeded, gasUsed);
+      blockchain.state().processBlock(finBlock2, succeeded, gasUsed, validatorUpdates);
 
       // Check that the transactions picked by the block builder each have the expected outcome.
       uint256_t accAbal2 = blockchain.state().getNativeBalance(accA.address);
@@ -493,21 +494,13 @@ namespace TBlockchain {
       std::vector<CometValidatorUpdate> validatorSet;
       uint64_t height = 0;
 
-      // Ensure all nodes think there are 4 validators for a while
-      while (height < 5) {
-        for (int i = 0; i < numNodes; ++i) {
-          blockchains[i]->getValidatorSet(validatorSet, height);
-          REQUIRE(validatorSet.size() == numValidators);
-        }
-        std::this_thread::sleep_for(std::chrono::milliseconds(100));
+      // Ensure all nodes see numValidator validators in the currently active validator set,
+      // which is the genesis set so it is immediately active.
+      for (int i = 0; i < numNodes; ++i) {
+        blockchains[i]->state().getValidatorSet(validatorSet, height);
+        REQUIRE(validatorSet.size() == numValidators);
       }
 
-      // FIXME: all validator set changes must be tracked as pending, then applied
-      //        this is only affecting the PENDING set
-      //        Blockchain should know precisely which validator set will come into effect when
-      //        For now, test this optimistic version (check directly with the Blockchain class)
-      //        THEN fix it so the Blockchain's view matches CometBFT's for each height.
-
       // TODO:
       // Chain owner stakes then delegates 5, 4, 3, 2, 1 tokens for nodes #0 .. #4.
       // After all transactions went through, validator set should be unchanged.
diff --git a/tests/integration/net/httpjsonrpc.cpp b/tests/integration/net/httpjsonrpc.cpp
index 7c8b73e8..6f943706 100644
--- a/tests/integration/net/httpjsonrpc.cpp
+++ b/tests/integration/net/httpjsonrpc.cpp
@@ -208,7 +208,7 @@ namespace THTTPJsonRPC {
       REQUIRE(eth_getBlockByHashResponse["result"]["transactionsRoot"] == newBestBlock.getTxMerkleRoot().hex(true));
       REQUIRE(eth_getBlockByHashResponse["result"]["stateRoot"] == Hash().hex(true));
       REQUIRE(eth_getBlockByHashResponse["result"]["receiptsRoot"] == Hash().hex(true));
-      REQUIRE(eth_getBlockByHashResponse["result"]["miner"] == sdk.validatorCometAddressToEthAddress(newBestBlock.getProposerAddr()).hex(true));
+      REQUIRE(eth_getBlockByHashResponse["result"]["miner"] == sdk.state().validatorCometAddressToEthAddress(newBestBlock.getProposerAddr()).hex(true));
       REQUIRE(eth_getBlockByHashResponse["result"]["difficulty"] == "0x1");
       REQUIRE(eth_getBlockByHashResponse["result"]["totalDifficulty"] == "0x1");
       REQUIRE(eth_getBlockByHashResponse["result"]["extraData"] == "0x0000000000000000000000000000000000000000000000000000000000000000");
@@ -249,7 +249,7 @@ namespace THTTPJsonRPC {
       REQUIRE(eth_getBlockByNumberResponse["result"]["transactionsRoot"] == newBestBlock.getTxMerkleRoot().hex(true));
       REQUIRE(eth_getBlockByNumberResponse["result"]["stateRoot"] == Hash().hex(true));
       REQUIRE(eth_getBlockByNumberResponse["result"]["receiptsRoot"] == Hash().hex(true));
-      REQUIRE(eth_getBlockByNumberResponse["result"]["miner"] == sdk.validatorCometAddressToEthAddress(newBestBlock.getProposerAddr()).hex(true));
+      REQUIRE(eth_getBlockByNumberResponse["result"]["miner"] == sdk.state().validatorCometAddressToEthAddress(newBestBlock.getProposerAddr()).hex(true));
       REQUIRE(eth_getBlockByNumberResponse["result"]["difficulty"] == "0x1");
       REQUIRE(eth_getBlockByNumberResponse["result"]["totalDifficulty"] == "0x1");
       REQUIRE(eth_getBlockByNumberResponse["result"]["extraData"] == "0x0000000000000000000000000000000000000000000000000000000000000000");

From 31626c68e6ac3a7c214165e136feaf972a5fb4cd Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Thu, 20 Feb 2025 13:37:10 -0300
Subject: [PATCH 10/15] WIP SystemContract unit test

---
 src/contract/executioncontext.cpp         |   2 +-
 src/contract/templates/systemcontract.cpp |  25 +-
 src/core/blockchain.h                     |   5 +-
 src/core/comet.cpp                        |  10 +-
 src/core/comet.h                          |  13 +-
 src/core/state.cpp                        |  17 +-
 src/core/state.h                          |   3 +-
 tests/integration/core/blockchain.cpp     | 300 +++++++++++++++++-----
 tests/sdktestsuite.cpp                    |   4 +-
 tests/sdktestsuite.hpp                    |  19 +-
 10 files changed, 293 insertions(+), 105 deletions(-)

diff --git a/src/contract/executioncontext.cpp b/src/contract/executioncontext.cpp
index 0e27fc08..296a2eaf 100644
--- a/src/contract/executioncontext.cpp
+++ b/src/contract/executioncontext.cpp
@@ -87,7 +87,7 @@ void ExecutionContext::transferBalance(View<Address> fromAddress, View<Address>
   auto recipient = getAccount(toAddress);
 
   if (sender.getBalance() < amount) {
-    throw DynamicException("insufficient founds");
+    throw DynamicException("ExecutionContext::transferBalance(): insufficient funds");
   }
 
   sender.setBalance(sender.getBalance() - amount);
diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index 298782b7..13d15a22 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -50,7 +50,10 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
   }
   // consider the vote deltas we have already accumulated in previous delegate/undelegate
   // txs processed before in this current block
-  checker += delegationDeltas_[validator];
+  auto dit = delegationDeltas_.find(validator);
+  if (dit != delegationDeltas_.end()) {
+    checker += dit->second;
+  }
   // consider the present vote delta being applied by the caller
   if (positive) {
     checker += delta;
@@ -67,7 +70,8 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
   // If we are violating this rule with the new voting power, return false so the
   //   caller can retry with another amount
   for (int i = 0; i < validatorVotes_.size(); ++i) {
-    if (validatorVotes_[i] == targetVotes) {
+    if (validators_[i] != validator && validatorVotes_[i] == targetVotes) {
+      LOGXTRACE("New delegation delta for validator " + Hex::fromBytes(validator).get() + " collides total votes: " + std::to_string(targetVotes));
       return false;
     }
   }
@@ -77,6 +81,7 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
   } else {
     delegationDeltas_[validator] -= delta;
   }
+  LOGXTRACE("New delegation delta for validator " + Hex::fromBytes(validator).get() + ": " + delegationDeltas_[validator].str());
   return true;
 }
 
@@ -169,7 +174,12 @@ void SystemContract::registerContractFunctions() {
 // TODO/REVIEW: rewrite as solidity deposit / fallback method?
 void SystemContract::stake() {
   // Tx native token value transferred is the staking amount, and tx sender is the depositor.
-  stakes_[this->getCaller()] += encodeAmount(this->getValue());
+  uint64_t amount = encodeAmount(this->getValue());
+  if (amount == 0) {
+    throw DynamicException("cannot deposit dust or zero");
+  }
+  stakes_[this->getCaller()] += amount;
+  LOGXTRACE("staked " + std::to_string(amount) + " for " + Hex::fromBytes(this->getCaller()).get() + ", current balance: " + std::to_string(stakes_[this->getCaller()]));
 }
 
 void SystemContract::unstake(const uint256_t& amount) {
@@ -197,6 +207,7 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
   PubKey validator = pubKeyFromString(validatorPubKey);
   const Address& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
+    LOGXTRACE("no stake entry for " + Hex::fromBytes(caller).get() + ", cannot delegate.");
     throw DynamicException("No stake");
   }
   uint64_t amount64 = encodeAmount(amount);
@@ -346,9 +357,11 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
 
     // clear irrelevant targetSlots_ entries created by unelected validators.
     // NOTE: it = erase(it) is not compiling, so we do a deferred erase-by-key loop.
+    // FIXME: targetSlots_.begin() (vs. cbegin()) causes a crash here, but it should work the same; must investigate.
+    //        NOTE: SafeUnorderedMap::iterator should be custom; should not expose the internal impl iterator.
     std::vector<PubKey> keysToErase;
-    auto it = targetSlots_.begin();
-    while (it != targetSlots_.end()) {
+    auto it = targetSlots_.cbegin();
+    while (it != targetSlots_.cend()) {
       if (!elected.contains(it->first)) {
         keysToErase.push_back(it->first);
       }
@@ -417,7 +430,7 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
         ++j;
       }
 
-      // If the new effective voting power for the validator change, generate an update
+      // If the new effective voting power for the validator changes, generate an update
       if (newVote != oldvv.votes) {
         LOGXTRACE("Validator update (existing): " + Hash(oldvv.validator.asBytes()).hex().get());
         validatorDeltas.push_back({oldvv.validator, newVote});
diff --git a/src/core/blockchain.h b/src/core/blockchain.h
index 8fbcb13e..5b8e263b 100644
--- a/src/core/blockchain.h
+++ b/src/core/blockchain.h
@@ -73,13 +73,12 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     const std::string instanceId_; ///< Instance ID for logging.
 
-    // NOTE: Comet *must* be destructed (stopped) before State, hence it must be declared after it.
-
+    // NOTE: Comet should be destructed (stopped) before State, hence it should be declared after it.
     Options options_; ///< Options singleton.
     Storage storage_; ///< BDK persistent store front-end.
     State state_;     ///< Blockchain state.
-    HTTPServer http_; ///< HTTP server.
     Comet comet_;     ///< CometBFT consensus engine driver.
+    HTTPServer http_; ///< HTTP server.
 
     // FIXME/TODO: Need to query for the last block and fill this in on boot.
     //             (That initial block query will also feed the fbCache_).
diff --git a/src/core/comet.cpp b/src/core/comet.cpp
index d6eb0aaf..cf1a30f8 100644
--- a/src/core/comet.cpp
+++ b/src/core/comet.cpp
@@ -241,8 +241,6 @@ class WebsocketRPCConnection : public Log::LogicalLocationProvider {
 
     /**
      * Make a synchronous (blocking) RPC call.
-     * Should use only when inspecting the cometbft node, otherwise should always use rpcAsyncCall() instead.
-     * NOTE: SLOW, polls with sleep, locks the entire RPC engine while waiting for its response.
      * @param method RPC method name to call.
      * @param params Parameters to pass to the RPC method.
      * @param outResult Outparam set to the json response to the request.
@@ -976,7 +974,7 @@ Bytes CometImpl::getValidatorPubKey() {
   return this->validatorPubKey_;
 }
 
-uint64_t CometImpl::sendTransaction(const Bytes& tx/*, std::shared_ptr<Hash>* ethHash*/) {
+uint64_t CometImpl::sendTransaction(const Bytes& tx) {
   // Add transaction to a queue that will try to push it to our cometbft instance.
   // Queue is NEVER reset on continue; etc. it is the same node, need to deliver tx to it eventually.
   // NOTE: sendTransaction is about queuing the tx object so that it is eventually dispatched to
@@ -998,7 +996,7 @@ uint64_t CometImpl::sendTransaction(const Bytes& tx/*, std::shared_ptr<Hash>* et
   std::string encodedTx = base64::encode_into<std::string>(tx.begin(), tx.end());
   json params = { {"tx", encodedTx} };
 
-  CometRPCRequestType requestData = TxSendType{/*ethHash ? *ethHash : nullptr,*/ tx};
+  CometRPCRequestType requestData = TxSendType{tx};
 
   uint64_t requestId = rpc_.rpcAsyncCall("broadcast_tx_async", params, requestData);
   return requestId;
@@ -2565,8 +2563,8 @@ Bytes Comet::getValidatorPubKey() {
   return impl_->getValidatorPubKey();
 }
 
-uint64_t Comet::sendTransaction(const Bytes& tx/*, std::shared_ptr<Hash>* ethHash*/) {
-  return impl_->sendTransaction(tx/*, ethHash*/);
+uint64_t Comet::sendTransaction(const Bytes& tx) {
+  return impl_->sendTransaction(tx);
 }
 
 uint64_t Comet::checkTransaction(const std::string& txHash) {
diff --git a/src/core/comet.h b/src/core/comet.h
index cda29402..3d6750a7 100644
--- a/src/core/comet.h
+++ b/src/core/comet.h
@@ -434,17 +434,12 @@ class Comet : public Log::LogicalLocationProvider {
      * If the ethash parameter is not provided, the transaction will not be added to the transaction
      * cache at the time of its creation.
      * @param tx The raw bytes of the transaction object.
-     * @param ethHash If ethHash is nullptr, won't compute the sha3 hash and won't track the transaction
-     * in the internal tx cache. If ethHash points to a shared_ptr that is nullptr, then it will create
-     * a shared Hash and store it in ethHash, and track the transaction in the tx cache. If ethHash
-     * points to a shared_ptr that is not nullptr, the function will assume the Hash value provided is
-     * the eth hash of tx and use it when storing the tx in the tx cache.
      * @return If > 0, the ticket number for the transaction send request (unique for one Comet object
-     * instantiation), or 0 if ethHash is not nullptr, the internal tx cache is enabled, and the
-     * transaction was not sent because there's already an entry for the transaction in the cache and
-     * its height is not CometTxStatusHeight::REJECTED (which allows for the transaction to be resent).
+     * instantiation), or 0 if the internal tx cache is enabled and the transaction was not sent
+     * because there's already an entry for the transaction in the cache and its height is not
+     * CometTxStatusHeight::REJECTED (which allows for the transaction to be resent).
      */
-    uint64_t sendTransaction(const Bytes& tx /*, std::shared_ptr<Hash>* ethHash = nullptr*/);
+    uint64_t sendTransaction(const Bytes& tx);
 
     /**
      * Enqueue a request to check the status of a transaction given its hash (CometBFT hash, i.e. SHA256).
diff --git a/src/core/state.cpp b/src/core/state.cpp
index 16081c23..2acf4445 100644
--- a/src/core/state.cpp
+++ b/src/core/state.cpp
@@ -139,7 +139,7 @@ void State::setValidators(const std::vector<CometValidatorUpdate>& newValidatorS
 }
 
 Address State::validatorCometAddressToEthAddress(Address validatorCometAddress) {
-  //std::shared_lock<std::shared_mutex> lock(stateMutex_);
+  std::shared_lock<std::shared_mutex> lock(stateMutex_);
   if (currentValidatorSet_ < 0) {
     throw DynamicException("No validator set: " + std::to_string(validatorSets_.size()));
   }
@@ -148,7 +148,7 @@ Address State::validatorCometAddressToEthAddress(Address validatorCometAddress)
 
 // Returns the currently active validator set and the height in which it became active
 void State::getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height) {
-  //std::shared_lock<std::shared_mutex> lock(stateMutex_);
+  std::shared_lock<std::shared_mutex> lock(stateMutex_);
   if (currentValidatorSet_ < 0) {
     throw DynamicException("No validator set: " + std::to_string(validatorSets_.size()));
   }
@@ -932,6 +932,14 @@ void State::processBlock(
   const FinalizedBlock& block, std::vector<bool>& succeeded, std::vector<uint64_t>& gasUsed,
   std::vector<CometValidatorUpdate>& validatorUpdates
 ) {
+  // NOTE: validatorCometAddressToEthAddress() locks the stateMutex_, so call it before we lock it again below.
+  //       (easier than creating a xxxInternal() version of it...)
+  // The coinbase address that gets all the block fees, etc. is the block proposer.
+  // Address derivation schemes (from the same Secp256k1 public key) differ between CometBFT and Eth.
+  // So we need to map CometBFT Address to CometValidatorUpdate (a validator public key)
+  //   and then use the validator public key to compute the correct Eth Address.
+  Address proposerEthAddr = blockchain_.state().validatorCometAddressToEthAddress(block.getProposerAddr());
+
   std::unique_lock lock(this->stateMutex_);
 
   // NOTE: Block should already have been validated by the caller.
@@ -954,11 +962,6 @@ void State::processBlock(
   ContractGlobals::blockHash_ = blockHash;
   ContractGlobals::blockHeight_ = blockHeight;
   ContractGlobals::blockTimestamp_ = block.getTimestamp();
-  // The coinbase address that gets all the block fees, etc. is the block proposer.
-  // Address derivation schemes (from the same Secp256k1 public key) differ between CometBFT and Eth.
-  // So we need to map CometBFT Address to CometValidatorUpdate (a validator public key)
-  //   and then use the validator public key to compute the correct Eth Address.
-  Address proposerEthAddr = blockchain_.state().validatorCometAddressToEthAddress(block.getProposerAddr());
   ContractGlobals::coinbase_ = proposerEthAddr;
   // LOGTRACE("Coinbase set to: " + proposerEthAddr.hex().get() + " (CometBFT Addr: " + block.getProposerAddr().hex().get() + ")");
 
diff --git a/src/core/state.h b/src/core/state.h
index d85c6394..a063f757 100644
--- a/src/core/state.h
+++ b/src/core/state.h
@@ -196,8 +196,7 @@ class State : public Log::LogicalLocationProvider {
     /**
      * Get the validator set that is currently in effect.
      * @param validatorSet Outparam with the validator set in effect.
-     * @param height Outparam with the block height in which this validator set took effect
-     * (past or present height) or will take effect (future height).
+     * @param height Outparam with the block height in which this validator set took effect.
      */
     void getValidatorSet(std::vector<CometValidatorUpdate>& validatorSet, uint64_t& height);
 
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index 5d86c744..9521a5dd 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -9,11 +9,114 @@ See the LICENSE.txt file in the project root for more information.
 #include "libs/base64.hpp"
 
 #include "core/blockchain.h"
+#include "contract/templates/systemcontract.h"
 
 #include "../../sdktestsuite.hpp"
 
 #include <filesystem>
 
+/**
+ * Create a test transaction that calls a CPP contract with some arguments.
+ */
+template <typename ReturnType, typename TContract, typename ...Args>
+void blockchainCallCpp(
+  Blockchain& blockchain,
+  const TestAccount& callerTestAccount,
+  const Address& contractAddress,
+  const uint256_t& value,
+  ReturnType(TContract::*func)(const Args&...),
+  const Args&... args
+) {
+  TContract::registerContract();
+  Functor txFunctor = ABI::FunctorEncoder::encode<Args...>(
+    ContractReflectionInterface::getFunctionName(func)
+  );
+  Bytes txData;
+  Utils::appendBytes(txData, UintConv::uint32ToBytes(txFunctor.value));
+  if constexpr (sizeof...(Args) > 0) {
+    Utils::appendBytes(
+      txData, ABI::Encoder::encodeData<Args...>(std::forward<decltype(args)>(args)...)
+    );
+  }
+  Gas gas(1'000'000'000);
+
+  //
+  // FIXME: I don't know why the estimateGas() function is not reverting.
+  //        It actually applies e.g. the stake or delegation, and it shows in logs.
+  //
+  //const uint64_t gasUsed = 10'000 + blockchain.state().estimateGas(
+  //  EncodedCallMessage(callerTestAccount.address, contractAddress, gas, value, txData)
+  //);
+  //const uint64_t gasUsed = 10'000 + std::invoke([&] () {
+  //  return blockchain.state().estimateGas(EncodedCallMessage(callerTestAccount.address, contractAddress, gas, value, txData));
+  //});
+  const uint64_t gasUsed = 1'000'000;  // Just use a big number for now
+
+  Bytes txBytes = TxBlock(
+    contractAddress,
+    callerTestAccount.address,
+    txData,
+    blockchain.opt().getChainID(),
+    blockchain.state().getNativeNonce(callerTestAccount.address),
+    value,
+    1000000000,
+    1000000000,
+    gasUsed,
+    callerTestAccount.privKey
+  ).rlpSerialize();
+  // Comet::sendTransaction() is fully asynchronous.
+  // If it returns 0, we know it failed, but if it returns > 0, we don't know if
+  //  the transaction will be accepted in the mempool, then included in a block,
+  //  and then executed successfully. The only way to know is to monitor the
+  //  blockchain's machine state for the expected transaction effects given a
+  //  certain timeout, or we would have to add some extra facilities to
+  //  Blockchain to make testing contracts easier.
+  REQUIRE(blockchain.comet().sendTransaction(txBytes) > 0);
+}
+
+void blockchainSendNativeTokens(
+  Blockchain& blockchain,
+  const TestAccount& fromAccount,
+  const Address& toAddress,
+  const uint256_t& value
+) {
+  Bytes txData;
+  Gas gas(1'000'000'000);
+  const uint64_t gasUsed = 10'000 + blockchain.state().estimateGas(
+    EncodedCallMessage(fromAccount.address, toAddress, gas, value, txData)
+  );
+  Bytes txBytes = TxBlock(
+    toAddress,
+    fromAccount.address,
+    txData,
+    blockchain.opt().getChainID(),
+    blockchain.state().getNativeNonce(fromAccount.address),
+    value,
+    1000000000,
+    1000000000,
+    gasUsed,
+    fromAccount.privKey
+  ).rlpSerialize();
+  REQUIRE(blockchain.comet().sendTransaction(txBytes) > 0);
+}
+
+/**
+ * Wait for a deposit to clear on a blockchain, or error out on timeout.
+ */
+bool blockchainCheckDeposit(
+  Blockchain& blockchain,
+  const Address& accountAddress,
+  const uint256_t& value
+) {
+  int tries = 1000; // func waitbal
+  while (--tries > 0) {
+    uint256_t bal = blockchain.state().getNativeBalance(accountAddress);
+    if (bal == value) { break; }
+    std::this_thread::sleep_for(std::chrono::milliseconds(10));
+  }
+  return tries > 0;
+}
+
 /**
  * Tests for the new Blockchain class.
  * Here we are using and testing the Blockchain class itself, not the SDKTestSuite test helper subclass.
@@ -23,7 +126,142 @@ namespace TBlockchain {
   TEST_CASE("Blockchain Class", "[integration][core][blockchain]") {
     std::string testDumpPath = Utils::getTestDumpPath();
 
-    // TODO: various Blockchain class RPC tests
+    SECTION("BlockchainValidatorSetTest") {
+      const int numNodes = 6;
+      const int numNonValidators = 2;
+      const int numValidators = numNodes - numNonValidators;
+
+      auto ports = SDKTestSuite::generateCometTestPorts(numNodes);
+
+      // Unfortunately, BDK HTTP ports were a late addition to getOptionsForTest()
+      std::vector<int> httpPorts;
+      for (int i = 0; i < numNodes; ++i) { httpPorts.push_back(SDKTestSuite::getTestPort()); }
+
+      std::vector<Options> options;
+      for (int i = 0; i < numNodes; ++i) {
+        options.emplace_back(
+          SDKTestSuite::getOptionsForTest(
+            createTestDumpPath("BlockchainValidatorSetTest_" + std::to_string(i)), false, "",
+            ports[i].p2p, ports[i].rpc, i, numNodes, ports, numNonValidators, 0,
+            "1s", "1s", httpPorts[i]
+          )
+        );
+      }
+
+      std::vector<std::unique_ptr<Blockchain>> blockchains;
+      for (int i = 0; i < numNodes; ++i) {
+        blockchains.emplace_back(std::make_unique<Blockchain>(options[i], std::to_string(i)));
+        // NOTE: Blockchain::start() waits for CometState::RUNNING, so it blocks for a while.
+        //       This is fine (the test still works, nodes eventually manage to dial each other) but
+        //       this could be parallelized so that the test would finish faster.
+        blockchains.back()->start();
+      }
+
+      std::vector<CometValidatorUpdate> origValidatorSet;
+      uint64_t origValidatorSetHeight = 0;
+
+      // Ensure all nodes see numValidator validators in the currently active validator set,
+      // which is the genesis set so it is immediately active.
+      for (int i = 0; i < numNodes; ++i) {
+        blockchains[i]->state().getValidatorSet(origValidatorSet, origValidatorSetHeight);
+        REQUIRE(origValidatorSet.size() == numValidators);
+      }
+
+      // From now on we are making some calls, so we need TestAccount for
+      // the chain owner and all the validators, which will be calling the
+      // SystemContract, staking tokens, etc.
+
+      // Create a TestAccount for each node based on its validator privkey
+      std::vector<TestAccount> nodeAccs;
+      for (int i = 0; i < numNodes; ++i) {
+        // We know that SDKTestSuite::getOptionsForTest() uses cometTestKeys
+        std::string privKeyStrBase64 = cometTestKeys[i].priv_key;
+        Bytes privBytes = base64::decode_into<Bytes>(privKeyStrBase64);
+        PrivKey privKey(privBytes);
+        nodeAccs.emplace_back(TestAccount(privKey));
+      }
+
+      Address systemContractAddr = ProtocolContractAddresses.at("SystemContract");
+      uint256_t aThousandNativeTokens = uint256_t("1000000000000000000000"); // 1'000 eth
+
+      for (int i = 0; i < numNodes; ++i) {
+        // Chain owner gives 5,000 tokens to each test node...
+        GLOGDEBUG("TEST: Send native tokens: " + std::to_string(i));
+        blockchainSendNativeTokens(
+          *blockchains[0], // use node 0 to call (could be any node)
+          SDKTestSuite::chainOwnerAccount(), // from: chain owner
+          nodeAccs[i].address, // to: each node's address (controlled by its validator private key)
+          aThousandNativeTokens * 5 // 5,000 eth
+        );
+        // ...and all nodes agree on this...
+        for (int j = 0; j < numNodes; ++j) {
+          REQUIRE(blockchainCheckDeposit(*blockchains[j], nodeAccs[i].address, aThousandNativeTokens * 5));
+        }
+        // ...and each test node stakes 1,000 tokens in the chain governance contract, so
+        //    each node can actually "register" (delegate to themselves).
+        blockchainCallCpp(
+          *blockchains[0], // use node 0 to call (could be any node)
+          nodeAccs[i], // from: node i
+          systemContractAddr, // to: SystemContract
+          aThousandNativeTokens, // node i is depositing (staking) this much in the SystemContract
+          &SystemContract::stake // calls SystemContract::stake() to deposit tokens from node i
+        );
+        // Balance in SystemContract account must grow by 1,000 eth for each validator account,
+        // and all running nodes agree on this.
+        for (int j = 0; j < numNodes; ++j) {
+          REQUIRE(blockchainCheckDeposit(*blockchains[j], systemContractAddr, aThousandNativeTokens * (i + 1)));
+        }
+      }
+
+      std::this_thread::sleep_for(std::chrono::milliseconds(3000)); // this should be more than enough
+      GLOGDEBUG("Test: start first delegations");
+
+      // Nodes #0 .. #4 delegate 5, 4, 3, 2, 1 for themselves, respectively
+      for (int i = 0; i < 5; ++i) {
+        blockchainCallCpp(
+          *blockchains[0], // use node 0 to call (could be any node)
+          nodeAccs[i], // node i is delegating
+          systemContractAddr, // to: SystemContract
+          0, // delegate does not send native tokens
+          &SystemContract::delegate, // calls SystemContract::delegate() to stake tokens on specific validator
+          Hex::fromBytes(nodeAccs[i].pubKey).get(), // the delegation is to node i (self)
+          uint256_t("1000000000000000000") * (5 - i) // node 0 delegates 5 eth, node 1 delegates 4 eth ... node 4 delegates 1 eth
+        );
+      }
+
+      // After all transactions went through, the validator set with 4 slots should be completely unchanged.
+      // NOTE: just wait a while and then read the blockchain's validator set, as the blockchain's validator set
+      //       is modified (or not) by the system contract when it receives delegate calls.
+      //       it could be that the system contract changed but the blockchain validator set wasn't notified, but
+      //       this check won't catch that kind of bug here.
+      std::this_thread::sleep_for(std::chrono::milliseconds(3000)); // this should be more than enough
+      GLOGDEBUG("Test: checking validator set did not change");
+      for (int i = 0; i < numNodes; ++i) {
+        std::vector<CometValidatorUpdate> validatorSet;
+        uint64_t validatorSetHeight = 0;
+        blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
+        REQUIRE(origValidatorSet.size() == validatorSet.size()); // == numValidators, already asserted above
+        for (int j = 0; j < validatorSet.size(); ++j) {
+          REQUIRE(origValidatorSet[j].publicKey == validatorSet[j].publicKey);
+          REQUIRE(origValidatorSet[j].power == validatorSet[j].power); // FIXME: this should FAIL, the power should have changed.
+        }
+        REQUIRE(origValidatorSetHeight == validatorSetHeight); // both should be 0 (genesis)
+      }
+
+      // TODO:
+      // Chain owner delegates 10 tokens for node #5
+      // After tx goes through, resulting validator set should be #5, #0, #1, #2 for the 4 slots.
+
+      // TODO:
+      // Validators #5, #0, and #1 vote to change the number of slots: 5, 5, 6
+      // After tx goes through, number of slots should change to 5, making the validator set be: #5, #0, #1, #2, #3
+
+      // TODO:
+      // Validator #1 gets fully undelegated (-4 tokens).
+      // After tx goes through, validator set should be #5, #0, #2, #3, #4
+
+      GLOGDEBUG("TEST: Validator set test finished.");
+    }
 
     SECTION("BlockchainBootTest") {
       std::string testDumpPath = createTestDumpPath("BlockchainBootTest");
@@ -459,66 +697,6 @@ namespace TBlockchain {
 
       GLOGDEBUG("TEST: done");
     }
-
-    SECTION("BlockchainValidatorSetTest") {
-      const int numNodes = 6;
-      const int numNonValidators = 2;
-      const int numValidators = numNodes - numNonValidators;
-
-      auto ports = SDKTestSuite::generateCometTestPorts(numNodes);
-
-      // Unfortunately, BDK HTTP ports were a late addition to getOptionsForTest()
-      std::vector<int> httpPorts;
-      for (int i = 0; i < numNodes; ++i) { httpPorts.push_back(SDKTestSuite::getTestPort()); }
-
-      std::vector<Options> options;
-      for (int i = 0; i < numNodes; ++i) {
-        options.emplace_back(
-          SDKTestSuite::getOptionsForTest(
-            createTestDumpPath("BlockchainValidatorSetTest_" + std::to_string(i)), false, "",
-            ports[i].p2p, ports[i].rpc, i, numNodes, ports, numNonValidators, 0,
-            "1s", "1s", httpPorts[i]
-          )
-        );
-      }
-
-      std::vector<std::unique_ptr<Blockchain>> blockchains;
-      for (int i = 0; i < numNodes; ++i) {
-        blockchains.emplace_back(std::make_unique<Blockchain>(options[i], std::to_string(i)));
-        // NOTE: Blockchain::start() waits for CometState::RUNNING, so it blocks for a while.
-        //       This is fine (the test still works, nodes eventually manage to dial each other) but
-        //       this could be parallelized so that the test would finish faster.
-        blockchains.back()->start();
-      }
-
-      std::vector<CometValidatorUpdate> validatorSet;
-      uint64_t height = 0;
-
-      // Ensure all nodes see numValidator validators in the currently active validator set,
-      // which is the genesis set so it is immediately active.
-      for (int i = 0; i < numNodes; ++i) {
-        blockchains[i]->state().getValidatorSet(validatorSet, height);
-        REQUIRE(validatorSet.size() == numValidators);
-      }
-
-      // TODO:
-      // Chain owner stakes then delegates 5, 4, 3, 2, 1 tokens for nodes #0 .. #4.
-      // After all transactions went through, validator set should be unchanged.
-
-      // TODO:
-      // Chain owner delegates 10 tokens for node #5
-      // After tx goes through, resulting validator set should be #5, #0, #1, #2 for the 4 slots.
-
-      // TODO:
-      // Validators #5, #0, and #1 vote to change the number of slots: 5, 5, 6
-      // After tx goes through, number of slots should change to 5, making the validator set be: #5, #0, #1, #2, #3
-
-      // TODO:
-      // Validator #1 gets fully undelegated (-4 tokens).
-      // After tx goes through, validator set should be #5, #0, #2, #3, #4
-
-      GLOGDEBUG("TEST: Validator set test finished.");
-    }
   }
 }
 
diff --git a/tests/sdktestsuite.cpp b/tests/sdktestsuite.cpp
index af73f1eb..6eb53ec0 100644
--- a/tests/sdktestsuite.cpp
+++ b/tests/sdktestsuite.cpp
@@ -380,8 +380,6 @@ Options SDKTestSuite::getOptionsForTest(
   std::string cometBFTTimeoutCommit,
   int bdkHttpPort
 ) {
-  // Note: all Comet instances are validators.
-
   // Sanity check arguments
   if (numKeys < 1 || numKeys > cometTestKeys.size() || keyNumber < 0 || keyNumber > numKeys - 1) {
     throw DynamicException("Invalid key arguments for getOptionsForCometTest().");
@@ -534,7 +532,7 @@ Options SDKTestSuite::getOptionsForTest(
     1,
     8080,
     Address(Hex::toBytes("0x00dead00665771855a34155f5e7405489df2c3c6")),
-    uint256_t(0),
+    uint256_t("1000000000000000000000000000"), // 1 billion eth for chain owner
     bdkHttpPort,
     2000,
     10000,
diff --git a/tests/sdktestsuite.hpp b/tests/sdktestsuite.hpp
index 2ed94cae..11bcc46b 100644
--- a/tests/sdktestsuite.hpp
+++ b/tests/sdktestsuite.hpp
@@ -55,6 +55,7 @@ struct CometTestPorts {
 /// Wrapper struct for accounts used within the SDKTestSuite.
 struct TestAccount {
   const PrivKey privKey;    ///< Private key of the account.
+  const PubKey pubKey;      ///< Public key of the account.
   const Address address;    ///< Address of the account.
   TestAccount() = default;  ///< Empty Account constructor.
 
@@ -62,7 +63,11 @@ struct TestAccount {
    * Account constructor.
    * @param privKey_ Private key of the account.
    */
-  TestAccount(const PrivKey& privKey_) : privKey(privKey_), address(Secp256k1::toAddress(Secp256k1::toPub(privKey))) {}
+  TestAccount(const PrivKey& privKey_)
+    : privKey(privKey_),
+      pubKey(Secp256k1::toPub(privKey)),
+      address(Secp256k1::toAddress(pubKey))
+  {}
 
   /// Create a new random account.
   inline static TestAccount newRandomAccount() { return TestAccount(PrivKey(Utils::randBytes(32))); }
@@ -77,11 +82,6 @@ struct TestAccount {
  */
 class SDKTestSuite : public Blockchain {
   private:
-    /// Owner of the chain (0x00dead00...).
-    static TestAccount chainOwnerAccount() {
-      return TestAccount(PrivKey(Hex::toBytes("0xe89ef6409c467285bcae9f80ab1cfeb3487cfe61ab28fb7d36443e1daa0c2867")));
-    };
-
     // Test listen P2P port number generator needs to be in SDKTestSuite due to createNewEnvironment(), which selects the port for the caller.
     // This should be used by all tests that open a node listen port, not only SDKTestSuite tests.
     static int p2pListenPortMin_;
@@ -99,6 +99,11 @@ class SDKTestSuite : public Blockchain {
     std::vector<TestAccount> testAccounts_;
 
   public:
+    /// Owner of the chain (0x00dead00...).
+    static TestAccount chainOwnerAccount() {
+      return TestAccount(PrivKey(Hex::toBytes("0xe89ef6409c467285bcae9f80ab1cfeb3487cfe61ab28fb7d36443e1daa0c2867")));
+    };
+
     /// Construct a test Blockchain.
     explicit SDKTestSuite(
       const Options& options,
@@ -407,7 +412,7 @@ class SDKTestSuite : public Blockchain {
       const Address& contractAddress,
       const uint256_t& value,
       const TestAccount& testAccount,
-      const uint64_t& timestamp,
+      const uint64_t& timestamp, // FIXME: This argument seems to be unused?
       ReturnType(TContract::*func)(const Args&...),
       const Args&... args
     ) {

From 35370e9fb16ed6e644d1b8481fb4cb6246eb79f9 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Thu, 20 Feb 2025 22:39:58 -0300
Subject: [PATCH 11/15] multiple fixes; WIP SystemContract & unit test

---
 src/contract/templates/systemcontract.cpp | 155 ++++++++++++++--------
 src/core/comet.cpp                        |   6 +-
 src/core/state.cpp                        |  19 ++-
 tests/integration/core/blockchain.cpp     |  11 +-
 4 files changed, 126 insertions(+), 65 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index 13d15a22..741e369c 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -25,7 +25,7 @@ struct ValidatorVotes {
   uint64_t  votes;
   bool operator<(const ValidatorVotes& other) const {
     if (votes != other.votes) {
-      return votes < other.votes;
+      return votes > other.votes; // Must actually sort biggest votes first
     }
     for (size_t i = 0; i < 33; ++i) { // 33 = PubKey byte size
       if (validator[i] != other.validator[i]) {
@@ -66,22 +66,24 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
     throw DynamicException("Delegation amount limit exceeded");
   }
   uint64_t targetVotes = checker.convert_to<uint64_t>();
+  // This is not needed as we imposed a total ordering based on public key binary comparison
+  //
   // No validator can have exactly the same voting power as any other
   // If we are violating this rule with the new voting power, return false so the
   //   caller can retry with another amount
-  for (int i = 0; i < validatorVotes_.size(); ++i) {
-    if (validators_[i] != validator && validatorVotes_[i] == targetVotes) {
-      LOGXTRACE("New delegation delta for validator " + Hex::fromBytes(validator).get() + " collides total votes: " + std::to_string(targetVotes));
-      return false;
-    }
-  }
+  //for (int i = 0; i < validatorVotes_.size(); ++i) {
+  //  if (validators_[i] != validator && validatorVotes_[i] == targetVotes) {
+  //    LOGDEBUG("New delegation delta for validator " + Hex::fromBytes(validator).get() + " collides total votes: " + std::to_string(targetVotes));
+  //    return false;
+  //  }
+  //}
   // All OK, so record it
   if (positive) {
     delegationDeltas_[validator] += delta;
   } else {
     delegationDeltas_[validator] -= delta;
   }
-  LOGXTRACE("New delegation delta for validator " + Hex::fromBytes(validator).get() + ": " + delegationDeltas_[validator].str());
+  LOGDEBUG("New delegation delta for validator " + Hex::fromBytes(validator).get() + ": " + delegationDeltas_[validator].str());
   return true;
 }
 
@@ -179,7 +181,7 @@ void SystemContract::stake() {
     throw DynamicException("cannot deposit dust or zero");
   }
   stakes_[this->getCaller()] += amount;
-  LOGXTRACE("staked " + std::to_string(amount) + " for " + Hex::fromBytes(this->getCaller()).get() + ", current balance: " + std::to_string(stakes_[this->getCaller()]));
+  LOGDEBUG("staked " + std::to_string(amount) + " for " + Hex::fromBytes(this->getCaller()).get() + ", current balance: " + std::to_string(stakes_[this->getCaller()]));
 }
 
 void SystemContract::unstake(const uint256_t& amount) {
@@ -207,7 +209,7 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
   PubKey validator = pubKeyFromString(validatorPubKey);
   const Address& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
-    LOGXTRACE("no stake entry for " + Hex::fromBytes(caller).get() + ", cannot delegate.");
+    LOGDEBUG("no stake entry for " + Hex::fromBytes(caller).get() + ", cannot delegate.");
     throw DynamicException("No stake");
   }
   uint64_t amount64 = encodeAmount(amount);
@@ -230,14 +232,16 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
     }
   }
   delegations_[caller][validator] += amount64;
-  // Loop avoiding collision in delegation amount (which we do not support)
-  while (!recordDelegationDelta(validator, amount64, true)) {
-    if (amount64 == 1) {
-      throw DynamicException("Cannot delegate this amount (try a larger amount)");
-    }
-    --amount64;
-    --delegations_[caller][validator];
-  }
+  // This is not needed as we imposed a total ordering based on public key binary comparison
+  // // Loop avoiding collision in delegation amount (which we do not support)
+  // while (!recordDelegationDelta(validator, amount64, true)) {
+  //  if (amount64 == 1) {
+  //    throw DynamicException("Cannot delegate this amount (try a larger amount)");
+  //  }
+  //  --amount64;
+  //  --delegations_[caller][validator];
+  //}
+  recordDelegationDelta(validator, amount64, true);
 }
 
 void SystemContract::undelegate(const std::string& validatorPubKey, const uint256_t& amount) {
@@ -269,14 +273,16 @@ void SystemContract::undelegate(const std::string& validatorPubKey, const uint25
   if (effectiveAmount > 0) {
     stakes_[caller] += effectiveAmount;
   }
-  // Loop avoiding collision in delegation amount (which we do not support)
-  while (!recordDelegationDelta(validator, amount64, false)) {
-    if (amount64 == 1) {
-      throw DynamicException("Cannot undelegate this amount (try a larger amount)");
-    }
-    --amount64;
-    ++delegations_[caller][validator];
-  }
+  // This is not needed as we imposed a total ordering based on public key binary comparison
+  // // Loop avoiding collision in delegation amount (which we do not support)
+  // while (!recordDelegationDelta(validator, amount64, false)) {
+  //   if (amount64 == 1) {
+  //     throw DynamicException("Cannot undelegate this amount (try a larger amount)");
+  //   }
+  //   --amount64;
+  //   ++delegations_[caller][validator];
+  // }
+  recordDelegationDelta(validator, amount64, false);
 }
 
 void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_t& slots) {
@@ -306,39 +312,67 @@ void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_
 }
 
 void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas) {
+
+  // Only run one of these at a time
+  static std::mutex printMutex; std::unique_lock<std::mutex> lk(printMutex);
+
   validatorDeltas.clear();
 
-  // feed updated validators_ & validatorVotes_ into a sorted validator,votes set
-  // save a copy of the validators_ & validatorVotes_ vector in old
-  std::set<ValidatorVotes> sorted;
+  LOGDEBUG("Enter finishBlock()");
+
+  // stores a copy of the validators_ & validatorVotes_ vector
   std::vector<ValidatorVotes> old;
+
+  // complete working map of all voted validators and their vote totals
+  std::map<PubKey, int256_t> votedValidators;
+
+  // This iterates over ALL validators that have votes (delegations).
+  // (including 0 votes, which is the special case for genesis validators).
   for (int i = 0; i < validators_.size(); ++i) {
+
+    // Working backup of validators_ and validatorVotes_
+    // `old` is used as basis to compute the validator update list that is sent to cometbft
     const auto& validator = validators_[i];
     ValidatorVotes vv;
     vv.validator = validator;
     vv.votes = validatorVotes_[i];
     old.push_back(vv);
-    if (validatorVotes_[i] > 0) {
-      auto it = delegationDeltas_.find(validator);
-      if (it != delegationDeltas_.end()) {
-        int256_t delta = it->second;
-        if (delta >= 0) {
-          vv.votes += delta.convert_to<uint64_t>();
-        } else {
-          delta = -delta;
-          vv.votes -= delta.convert_to<uint64_t>();
-        }
-      }
-      sorted.insert(vv);
-    }
+
+    // intialize the global vote map with all validators that already had votes
+    votedValidators[validator] = validatorVotes_[i];
   }
 
-  bool changedDelegations = !delegationDeltas_.empty();
+  // Apply all the vote deltas for this block on top of the voting totals that were already in effect.
+  // If a validator did not have any votes, a new key will be inserted in votedValidators.
+  auto dit = delegationDeltas_.cbegin();
+  while (dit != delegationDeltas_.cend()) {
+    votedValidators[dit->first] += dit->second;
+    ++dit;
+  }
+
+  // Use votedValidators to build the `sorted` set, which will sort the
+  // validators based on voting power with a total order, breaking ties
+  // on the validator public key content.
+  std::set<ValidatorVotes> sorted;
+  for (const auto& [validator, votes] : votedValidators) {
+    sorted.insert(
+      ValidatorVotes{
+        validator,
+        static_cast<uint64_t>(votes)
+      }
+    );
+  }
 
   // If delegations are unchanged, no need to fix validators_ & validatorVotes_
+  bool changedDelegations = !delegationDeltas_.empty();
   if (changedDelegations) {
+    LOGDEBUG("Got delegationDeltas_: " + std::to_string(delegationDeltas_.size()));
+
+    // done using delegation deltas, so clear them for the next block
+    delegationDeltas_.clear();
 
     // rebuild validators_ & validatorVotes_
+    // computes an ancillary `elected` set for targetSlots_ recalculation
     validators_.clear();
     validatorVotes_.clear();
     std::set<PubKey> elected;
@@ -347,14 +381,12 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
       validators_.push_back(vv.validator);
       validatorVotes_.push_back(vv.votes);
       if (i < numSlots_.get()) {
+        LOGDEBUG("Elected: " + Hex::fromBytes(vv.validator).get());
         elected.insert(vv.validator);
       }
       ++i;
     }
 
-    // clear delegation deltas
-    delegationDeltas_.clear();
-
     // clear irrelevant targetSlots_ entries created by unelected validators.
     // NOTE: it = erase(it) is not compiling, so we do a deferred erase-by-key loop.
     // FIXME: targetSlots_.begin() (vs. cbegin()) causes a crash here, but it should work the same; must investigate.
@@ -384,15 +416,18 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
   bool changedSlots = false;
   while (sit != sorted.end() && vIdx < numSlots_.get()) {
     ++electedValidatorCount;
-    int64_t slotsVote = static_cast<int64_t>(targetSlots_[sit->validator]); // cast is OK since slotsVote < maxSlots_
-    if (slotsVote > numSlots_.get()) {
-      incVote = std::min(incVote, slotsVote);
-      incVote = std::min(incVote, static_cast<int64_t>(maxSlots_.get())); // ensure whatever maxSlots_ is cannot be exceeded
-      ++incVoteCount;
-    } else if (slotsVote < numSlots_.get()) {
-      decVote = std::max(decVote, slotsVote);
-      // already protected against slotsVote == 0 on vote submission
-      ++decVoteCount;
+    auto tit = targetSlots_.find(sit->validator);
+    if (tit != targetSlots_.end()) {
+      int64_t slotsVote = static_cast<int64_t>(tit->second); // cast is OK since slotsVote < maxSlots_
+      if (slotsVote > numSlots_.get()) {
+        incVote = std::min(incVote, slotsVote);
+        incVote = std::min(incVote, static_cast<int64_t>(maxSlots_.get())); // ensure whatever maxSlots_ is cannot be exceeded
+        ++incVoteCount;
+      } else if (slotsVote < numSlots_.get()) {
+        decVote = std::max(decVote, slotsVote);
+        // already protected against slotsVote == 0 on vote submission
+        ++decVoteCount;
+      }
     }
     ++sit;
     ++vIdx;
@@ -401,11 +436,11 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
   if (incVoteCount >= quorum && incVote != numSlots_.get()) {
     numSlots_ = static_cast<uint64_t>(incVote);
     changedSlots = true;
-    LOGXTRACE("Increased numSlots_: " + std::to_string(numSlots_.get()));
+    LOGDEBUG("Increased numSlots_: " + std::to_string(numSlots_.get()));
   } else if (decVoteCount >= quorum && decVote != numSlots_.get()) {
     numSlots_ = static_cast<uint64_t>(decVote);
     changedSlots = true;
-    LOGXTRACE("Decreased numSlots_: " + std::to_string(numSlots_.get()));
+    LOGDEBUG("Decreased numSlots_: " + std::to_string(numSlots_.get()));
   }
 
   // Finally, compute the CometBFT validator updates if any.
@@ -432,7 +467,7 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
 
       // If the new effective voting power for the validator changes, generate an update
       if (newVote != oldvv.votes) {
-        LOGXTRACE("Validator update (existing): " + Hash(oldvv.validator.asBytes()).hex().get());
+        LOGDEBUG("Validator update (existing): " + Hex::fromBytes(oldvv.validator).get());
         validatorDeltas.push_back({oldvv.validator, newVote});
       }
     }
@@ -461,11 +496,13 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
       }
       if (!found) {
         // vv.validator has >0 power now, but had ==0 power before (not elected)
-        LOGXTRACE("Validator update (new): " + Hash(vv.validator.asBytes()).hex().get());
+        LOGDEBUG("Validator update (new): " + Hex::fromBytes(vv.validator).get());
         validatorDeltas.push_back({vv.validator, vv.votes});
       }
     }
   }
+
+  LOGDEBUG("Exit finishBlock()");
 }
 
 DBBatch SystemContract::dump() const {
diff --git a/src/core/comet.cpp b/src/core/comet.cpp
index cf1a30f8..bf6f36bb 100644
--- a/src/core/comet.cpp
+++ b/src/core/comet.cpp
@@ -1331,7 +1331,9 @@ void CometImpl::doStartCometBFT(
         GLOGXTRACE("[cometbft stdout]: " + line);
         *processStdout += line + '\n';
       } else {
-        GLOGDEBUG("[cometbft stdout]: " + line);
+        // Leaving cometbft output to TRACE is probably optimal.
+        // This makes DEBUG logs far less verbose (when the bug is unrelated to consensus traffic).
+        GLOGTRACE("[cometbft stdout]: " + line);
       }
     }
     // remove trailing \n so that e.g. the node id from cometbft show-node-id is exactly processStdout without a need to trim it.
@@ -1346,6 +1348,8 @@ void CometImpl::doStartCometBFT(
         GLOGXTRACE("[cometbft stderr]: " + line);
         *processStderr += line + '\n';
       } else {
+        // If cometbft generates stderr messages, we probably want to see it during regular debugging,
+        // even if we aren't tracing normal cometbft output.
         GLOGDEBUG("[cometbft stderr]: " + line);
       }
     }
diff --git a/src/core/state.cpp b/src/core/state.cpp
index 2acf4445..01fce049 100644
--- a/src/core/state.cpp
+++ b/src/core/state.cpp
@@ -130,6 +130,10 @@ void State::setValidators(const std::vector<CometValidatorUpdate>& newValidatorS
     // Point to the genesis set we are creating.
     currentValidatorSet_ = 0;
   }
+  LOGDEBUG("NEW VALIDATOR SET: " + std::to_string(activationHeight));
+  for (const auto& v : newValidatorSet) {
+    LOGDEBUG("  " + Hex::fromBytes(v.publicKey).get() + " = " + std::to_string(v.power));
+  }
   validatorSets_.emplace_front(
     ValidatorSet(
       activationHeight,
@@ -984,8 +988,8 @@ void State::processBlock(
   std::vector<std::pair<PubKey, uint64_t>> validatorDeltas;
   SystemContract* systemContractPtr = getSystemContractInternal();
   systemContractPtr->finishBlock(validatorDeltas); // Collect any validator changes accumulated in the singleton system contract...
-
   if (!validatorDeltas.empty()) {
+    LOGDEBUG("Got validatorDeltas: " + std::to_string(validatorDeltas.size()));
     // Apply validator set deltas
     Bytes validatorDbLog;
     for (const auto& validatorDelta : validatorDeltas) {
@@ -1023,9 +1027,17 @@ void State::processBlock(
     setValidators(newValidatorSet);
   }
 
+  // *********************************************************
+  // FIXME: validatorsets_ is not working
+  // -- print "processblock() height == X"
+  // -- print validatorsets_ size and activation heights of all elements [x,y,z...]
+  // -- print what you are doing to the currentvalidatorset_ and the activation height that it is pointing to
+  // why is getvalidatorset returning 0 and not the new one?
+  // *********************************************************
+
   // Since we have advanced the simulation height, we may need to prune validatorSets_
   // Reverse search for the first validator set that is pending, and for each iteration,
-  //   add 1 to the count of old validatorSets_ entires to prune.
+  //   add 1 to the count of old validatorSets_ entries to prune.
   // Example: if no pending validator sets exist (all are active), will prune all but the front.
   // Example: [pending pending active active active] should prune the last 2.
   int pruneCount = -1; // Start at -1 so the last active is preserved.
@@ -1039,6 +1051,9 @@ void State::processBlock(
     validatorSets_.pop_back();
     --pruneCount;
   }
+
+  // After pruning validatorSets_, the currentValidatorSet_ is simply the last one.
+  currentValidatorSet_ = validatorSets_.size() - 1;
 }
 
 void State::processTransaction(
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index 9521a5dd..7af096d0 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -234,14 +234,18 @@ namespace TBlockchain {
       //       is modified (or not) by the system contract when it receives delegate calls.
       //       it could be that the system contract changed but the blockchain validator set wasn't notified, but
       //       this check won't catch that kind of bug here.
-      std::this_thread::sleep_for(std::chrono::milliseconds(3000)); // this should be more than enough
+      GLOGDEBUG("Test: end first delegations");
+      std::this_thread::sleep_for(std::chrono::milliseconds(10'000)); // we actually need to wait for H+2 activation!
       GLOGDEBUG("Test: checking validator set did not change");
-      for (int i = 0; i < numNodes; ++i) {
+      for (int i = 0; i < 1; ++i) { // FIXME: to 5
         std::vector<CometValidatorUpdate> validatorSet;
         uint64_t validatorSetHeight = 0;
         blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
         REQUIRE(origValidatorSet.size() == validatorSet.size()); // == numValidators, already asserted above
+        GLOGDEBUG("COMPARE VALIDATOR SET: old vs new activation height: " + std::to_string(origValidatorSetHeight) + " --> " + std::to_string(validatorSetHeight));
         for (int j = 0; j < validatorSet.size(); ++j) {
+          GLOGDEBUG("  ORIG: " + Hex::fromBytes(origValidatorSet[j].publicKey).get() + " = " + std::to_string(origValidatorSet[j].power));
+          GLOGDEBUG("  NEW : " + Hex::fromBytes(validatorSet[j].publicKey).get() + " = " + std::to_string(validatorSet[j].power));
           REQUIRE(origValidatorSet[j].publicKey == validatorSet[j].publicKey);
           REQUIRE(origValidatorSet[j].power == validatorSet[j].power); // FIXME: this should FAIL, the power should have changed.
         }
@@ -262,7 +266,7 @@ namespace TBlockchain {
 
       GLOGDEBUG("TEST: Validator set test finished.");
     }
-
+/*
     SECTION("BlockchainBootTest") {
       std::string testDumpPath = createTestDumpPath("BlockchainBootTest");
 
@@ -697,6 +701,7 @@ namespace TBlockchain {
 
       GLOGDEBUG("TEST: done");
     }
+*/
   }
 }
 

From 91cdc60950ebd4cd0ebe816f230eaacc0ffa21eb Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Fri, 21 Feb 2025 16:02:49 -0300
Subject: [PATCH 12/15] finish SystemContract and unit test + cleanups, fixes,
 docs

---
 src/bins/faucet-api/src/httpserver.cpp    |   4 +-
 src/contract/templates/systemcontract.cpp | 172 ++++++++-------
 src/contract/templates/systemcontract.h   |   4 +
 src/core/blockchain.cpp                   |  24 ++
 src/core/blockchain.h                     |  22 ++
 src/core/comet.cpp                        | 115 ++++++----
 src/core/state.cpp                        |  31 +--
 src/net/abci/abciserver.cpp               |   4 +-
 src/net/http/httpserver.cpp               |   4 +-
 tests/integration/core/blockchain.cpp     | 256 ++++++++++++++++++++--
 tests/sdktestsuite.hpp                    |   3 +
 11 files changed, 464 insertions(+), 175 deletions(-)

diff --git a/src/bins/faucet-api/src/httpserver.cpp b/src/bins/faucet-api/src/httpserver.cpp
index 5a3b8022..15fe531a 100644
--- a/src/bins/faucet-api/src/httpserver.cpp
+++ b/src/bins/faucet-api/src/httpserver.cpp
@@ -20,12 +20,12 @@ namespace Faucet {
     std::vector<std::thread> v;
     v.reserve(4 - 1);
     for (int i = 4 - 1; i > 0; i--) v.emplace_back([&]{ this->ioc_.run(); });
-    LOGINFO(std::string("HTTP Server Started at port: ") + std::to_string(port_));
+    LOGTRACE(std::string("HTTP server started at port: ") + std::to_string(port_));
     this->ioc_.run();
 
     // If we get here, it means we got a SIGINT or SIGTERM. Block until all the threads exit
     for (std::thread& t : v) t.join();
-    LOGINFO("HTTP Server Stopped");
+    LOGTRACE("HTTP server stopped");
     return true;
   }
 
diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index 741e369c..50b0ec0c 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -66,24 +66,12 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
     throw DynamicException("Delegation amount limit exceeded");
   }
   uint64_t targetVotes = checker.convert_to<uint64_t>();
-  // This is not needed as we imposed a total ordering based on public key binary comparison
-  //
-  // No validator can have exactly the same voting power as any other
-  // If we are violating this rule with the new voting power, return false so the
-  //   caller can retry with another amount
-  //for (int i = 0; i < validatorVotes_.size(); ++i) {
-  //  if (validators_[i] != validator && validatorVotes_[i] == targetVotes) {
-  //    LOGDEBUG("New delegation delta for validator " + Hex::fromBytes(validator).get() + " collides total votes: " + std::to_string(targetVotes));
-  //    return false;
-  //  }
-  //}
   // All OK, so record it
   if (positive) {
     delegationDeltas_[validator] += delta;
   } else {
     delegationDeltas_[validator] -= delta;
   }
-  LOGDEBUG("New delegation delta for validator " + Hex::fromBytes(validator).get() + ": " + delegationDeltas_[validator].str());
   return true;
 }
 
@@ -91,7 +79,7 @@ SystemContract::SystemContract(
   const Address& address, const DB& db
 ) : DynamicContract(address, db)
 {
-  LOGDEBUG("Loading SystemContract...");
+  LOGTRACE("Loading SystemContract...");
 
   this->numSlots_ = Utils::fromBigEndian<uint64_t>(db.get(std::string("numSlots_"), this->getDBPrefix()));
   this->numSlots_.commit();
@@ -121,8 +109,6 @@ SystemContract::SystemContract(
   delegationDeltas_.clear();
   delegationDeltas_.commit();
   doRegister();
-
-  LOGDEBUG("Loaded SystemContract.");
 }
 
 SystemContract::SystemContract(
@@ -131,6 +117,8 @@ SystemContract::SystemContract(
   const Address& address, const Address& creator, const uint64_t& chainId
 ) : DynamicContract("SystemContract", address, creator, chainId)
 {
+  LOGTRACE("Creating SystemContract...");
+
   std::vector<PubKey> initialValidators;
   for (const auto& pubKeyStr : initialValidatorPubKeys) {
     initialValidators.push_back(pubKeyFromString(pubKeyStr));
@@ -181,7 +169,6 @@ void SystemContract::stake() {
     throw DynamicException("cannot deposit dust or zero");
   }
   stakes_[this->getCaller()] += amount;
-  LOGDEBUG("staked " + std::to_string(amount) + " for " + Hex::fromBytes(this->getCaller()).get() + ", current balance: " + std::to_string(stakes_[this->getCaller()]));
 }
 
 void SystemContract::unstake(const uint256_t& amount) {
@@ -209,7 +196,6 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
   PubKey validator = pubKeyFromString(validatorPubKey);
   const Address& caller = this->getCaller();
   if (stakes_.find(caller) == stakes_.end()) {
-    LOGDEBUG("no stake entry for " + Hex::fromBytes(caller).get() + ", cannot delegate.");
     throw DynamicException("No stake");
   }
   uint64_t amount64 = encodeAmount(amount);
@@ -309,16 +295,16 @@ void SystemContract::voteSlots(const std::string& validatorPubKey, const uint64_
   }
   // Save numslots choice
   targetSlots_[validator] = slots;
+  targetSlotsModified_ = true;
 }
 
 void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& validatorDeltas) {
-
-  // Only run one of these at a time
-  static std::mutex printMutex; std::unique_lock<std::mutex> lk(printMutex);
-
-  validatorDeltas.clear();
-
-  LOGDEBUG("Enter finishBlock()");
+  // if no delegation deltas and no numslots-change votes,
+  // then it is guraanteed that there's nothing to do
+  bool changedDelegations = !delegationDeltas_.empty();
+  if (!changedDelegations && !targetSlotsModified_) {
+    return;
+  }
 
   // stores a copy of the validators_ & validatorVotes_ vector
   std::vector<ValidatorVotes> old;
@@ -364,9 +350,7 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
   }
 
   // If delegations are unchanged, no need to fix validators_ & validatorVotes_
-  bool changedDelegations = !delegationDeltas_.empty();
   if (changedDelegations) {
-    LOGDEBUG("Got delegationDeltas_: " + std::to_string(delegationDeltas_.size()));
 
     // done using delegation deltas, so clear them for the next block
     delegationDeltas_.clear();
@@ -381,7 +365,6 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
       validators_.push_back(vv.validator);
       validatorVotes_.push_back(vv.votes);
       if (i < numSlots_.get()) {
-        LOGDEBUG("Elected: " + Hex::fromBytes(vv.validator).get());
         elected.insert(vv.validator);
       }
       ++i;
@@ -389,6 +372,8 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
 
     // clear irrelevant targetSlots_ entries created by unelected validators.
     // NOTE: it = erase(it) is not compiling, so we do a deferred erase-by-key loop.
+    // NOTE: deleting targetSlots_ entries does not need to set targetSlotsModified_ = true, since you can't cause
+    //       a numSlots_ change by removing targetSlots_ entries.
     // FIXME: targetSlots_.begin() (vs. cbegin()) causes a crash here, but it should work the same; must investigate.
     //        NOTE: SafeUnorderedMap::iterator should be custom; should not expose the internal impl iterator.
     std::vector<PubKey> keysToErase;
@@ -404,43 +389,70 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
     }
   }
 
-  // Fully reevaluate targetSlots_ to see if numSlots_ changes.
-  uint64_t oldNumSlots = numSlots_.get(); // Save oldNumSlots since numSlots_ may change
-  int64_t incVote = std::numeric_limits<int64_t>::max();
-  int64_t decVote = std::numeric_limits<int64_t>::min();
-  uint64_t incVoteCount = 0;
-  uint64_t decVoteCount = 0;
-  int vIdx = 0;
-  auto sit = sorted.begin();
-  uint64_t electedValidatorCount = 0;
   bool changedSlots = false;
-  while (sit != sorted.end() && vIdx < numSlots_.get()) {
-    ++electedValidatorCount;
-    auto tit = targetSlots_.find(sit->validator);
-    if (tit != targetSlots_.end()) {
-      int64_t slotsVote = static_cast<int64_t>(tit->second); // cast is OK since slotsVote < maxSlots_
-      if (slotsVote > numSlots_.get()) {
-        incVote = std::min(incVote, slotsVote);
-        incVote = std::min(incVote, static_cast<int64_t>(maxSlots_.get())); // ensure whatever maxSlots_ is cannot be exceeded
-        ++incVoteCount;
-      } else if (slotsVote < numSlots_.get()) {
-        decVote = std::max(decVote, slotsVote);
-        // already protected against slotsVote == 0 on vote submission
-        ++decVoteCount;
+  uint64_t oldNumSlots = numSlots_.get(); // Save oldNumSlots since numSlots_ may change
+  if (targetSlotsModified_) {
+    targetSlotsModified_ = false;
+    targetSlotsModified_.commit(); // This is not a contract/machine call (not in a transaction context) so we need to force the change.
+
+    // Reevaluate targetSlots_ to see if numSlots_ changes.
+    std::multiset<int64_t> incVoteSet;
+    std::multiset<int64_t> decVoteSet;
+    int vIdx = 0;
+    auto sit = sorted.begin();
+    uint64_t electedValidatorCount = 0;
+    while (sit != sorted.end() && vIdx < numSlots_.get()) {
+      ++electedValidatorCount;
+      auto tit = targetSlots_.find(sit->validator);
+      if (tit != targetSlots_.end()) {
+        int64_t slotsVote = static_cast<int64_t>(tit->second); // cast is OK since slotsVote < maxSlots_
+        if (slotsVote > numSlots_.get()) {
+          slotsVote = std::min(slotsVote, static_cast<int64_t>(maxSlots_.get())); // ensure whatever maxSlots_ is cannot be exceeded
+          incVoteSet.insert(slotsVote);
+        } else if (slotsVote < numSlots_.get()) {
+          decVoteSet.insert(slotsVote);
+        }
+      }
+      ++sit;
+      ++vIdx;
+    }
+    uint64_t quorum = ((electedValidatorCount * 2) / 3) + 1;
+    if (incVoteSet.size() >= quorum) {
+      auto qit = incVoteSet.rbegin(); //smallest number to increase to is prioritized
+      for (int i = 1; i < quorum; ++i) {
+        ++qit;
+        if (qit == incVoteSet.rend()) {
+          throw DynamicException("SystemContract fatal error while calculating new validator slot count");
+        }
+      }
+      numSlots_ = static_cast<uint64_t>(*qit);
+      changedSlots = true;
+    } else if (decVoteSet.size() >= quorum) {
+      auto qit = decVoteSet.begin(); //largest number to decrease to is prioritized
+      for (int i = 1; i < quorum; ++i) {
+        ++qit;
+        if (qit == decVoteSet.end()) {
+          throw DynamicException("SystemContract fatal error while calculating new validator slot count");
+        }
+      }
+      numSlots_ = static_cast<uint64_t>(*qit);
+      changedSlots = true;
+    }
+
+    if (changedSlots) {
+      // Remove all slots votes that were precisely satisfied
+      std::set<PubKey> keysToRemove;
+      auto tit = targetSlots_.cbegin();
+      while (tit != targetSlots_.cend()) {
+        if (tit->second == numSlots_.get()) {
+          keysToRemove.insert(tit->first);
+        }
+        ++tit;
+      }
+      for (const auto& key : keysToRemove) {
+        targetSlots_.erase(key);
       }
     }
-    ++sit;
-    ++vIdx;
-  }
-  uint64_t quorum = ((electedValidatorCount * 2) / 3) + 1;
-  if (incVoteCount >= quorum && incVote != numSlots_.get()) {
-    numSlots_ = static_cast<uint64_t>(incVote);
-    changedSlots = true;
-    LOGDEBUG("Increased numSlots_: " + std::to_string(numSlots_.get()));
-  } else if (decVoteCount >= quorum && decVote != numSlots_.get()) {
-    numSlots_ = static_cast<uint64_t>(decVote);
-    changedSlots = true;
-    LOGDEBUG("Decreased numSlots_: " + std::to_string(numSlots_.get()));
   }
 
   // Finally, compute the CometBFT validator updates if any.
@@ -451,12 +463,17 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
     for (int i = 0; i < old.size() && i < oldNumSlots; ++i) {
       const auto& oldvv = old[i];
 
-      // For each old elected validator, figure out its new voting power (which is zero if its rank/index
-      //  is higher than the potentially new numSlots_ limit).
-      uint64_t newVote = 0;
+      // For each old elected validator, figure out its *effective* new voting power
+      uint64_t newVote = 0; // (the effective new voting power is zero if the validator is sorted past the numSlots_ limit).
       int j = 0;
       auto it = sorted.begin();
       while (it != sorted.end() && j < numSlots_.get()) {
+        // also note that if a validator candidate is fully undelegated (receives 0 votes)
+        // it will simply disappear from validators_ / validatorVotes_ / sorted / etc.
+        // and so this will also guaranteed not match, resulting in newVote == 0 and
+        // if that's different than the voting power it had before, it will generate a
+        // validatorDeltas update below with a voting power of 0 (power 0 update is
+        // interpreted by CometBFT as "remove this validator from consensus").
         if (it->validator == oldvv.validator) {
           newVote = it->votes;
           break;
@@ -467,46 +484,47 @@ void SystemContract::finishBlock(std::vector<std::pair<PubKey, uint64_t>>& valid
 
       // If the new effective voting power for the validator changes, generate an update
       if (newVote != oldvv.votes) {
-        LOGDEBUG("Validator update (existing): " + Hex::fromBytes(oldvv.validator).get());
         validatorDeltas.push_back({oldvv.validator, newVote});
       }
     }
 
-    // Generate updates for fresh elected validators (validators that had
-    // zero votes before this block and are now elected).
+    // Generate updates for fresh elected validators (validators that were not elected
+    //  before this block was processed, but are now elected for whatever reason).
     int k = 0;
     for (const auto& vv : sorted) {
+      // ensure vv is an elected validator
       if (k >= numSlots_.get()) {
         // exceeded numSlots_, reached the voted but unelected validator range
         break;
       }
       ++k;
-      bool found = false;
+
+      // ensure vv, which at this point we know is elected presently, was not elected before in old
+      bool wasAlreadyElected = false;
       int l = 0;
       for (const auto& oldvv : old) {
         if (l >= oldNumSlots) {
-          // exceeded oldNumSlots, reached the voted but unelected validator range
+          // exceeded oldNumSlots, reached the voted but unelected validator range of old
           break;
         }
         ++l;
         if (oldvv.validator == vv.validator) {
-          found = true;
+          wasAlreadyElected = true;
           break;
         }
       }
-      if (!found) {
-        // vv.validator has >0 power now, but had ==0 power before (not elected)
-        LOGDEBUG("Validator update (new): " + Hex::fromBytes(vv.validator).get());
+      if (!wasAlreadyElected) {
+        // vv.validator is elected now, but was not elected before
+        // remember that CometBFT validator updates with positive power mean
+        // that we do include the validator in the active validator set.
         validatorDeltas.push_back({vv.validator, vv.votes});
       }
     }
   }
-
-  LOGDEBUG("Exit finishBlock()");
 }
 
 DBBatch SystemContract::dump() const {
-  LOGDEBUG("Saving SystemContract...");
+  LOGTRACE("Saving SystemContract...");
 
   DBBatch batch = BaseContract::dump();
   batch.push_back(StrConv::stringToBytes("numSlots_"), UintConv::uint64ToBytes(this->numSlots_.get()), this->getDBPrefix());
@@ -536,7 +554,5 @@ DBBatch SystemContract::dump() const {
     // There is *never* a valid reason to save an inconsistent state snapshot in the middle of block processing.
     throw DynamicException("System contract is in an inconsistent state during snapshotting (delegationDeltas_ is not empty).");
   }
-
-  LOGDEBUG("Saved SystemContract.");
   return batch;
 }
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index fe5835f4..dc2f9733 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -13,6 +13,7 @@ See the LICENSE.txt file in the project root for more information.
 #include "../variables/safeunorderedmap.h"
 #include "../variables/safevector.h"
 #include "../variables/safeint.h"
+#include "../variables/safebool.h"
 
 /**
  * Chain governor contract
@@ -82,6 +83,9 @@ class SystemContract : public DynamicContract {
     /// targetSlots_, then that number will be the new numSlots_.
     SafeUnorderedMap<PubKey, uint64_t> targetSlots_;
 
+    /// Optimization: avoid recomputing targetSlots_ if no voteSlots() called.
+    SafeBool targetSlotsModified_;
+
     /// Liquid deposits by users.
     SafeUnorderedMap<Address, uint64_t> stakes_;
 
diff --git a/src/core/blockchain.cpp b/src/core/blockchain.cpp
index 1588246c..a6a6263d 100644
--- a/src/core/blockchain.cpp
+++ b/src/core/blockchain.cpp
@@ -122,6 +122,25 @@ Blockchain::Blockchain(const Options& options, std::string instanceId)
 {
 }
 
+void Blockchain::lockBlockProcessing() {
+  std::scoped_lock lock(incomingBlockLockMutex_);
+  incomingBlockLock_ = true;
+}
+
+void Blockchain::unlockBlockProcessing() {
+  incomingBlockLock_ = false;
+}
+
+int Blockchain::getNumUnconfirmedTxs() {
+  json ret;
+  if (comet_.rpcSyncCall("num_unconfirmed_txs", {}, ret)) {
+    if (ret.contains("result") && ret["result"].contains("n_txs")) {
+      return std::stoi(ret["result"]["n_txs"].get<std::string>());
+    }
+  }
+  return -1;
+}
+
 bool Blockchain::getBlockRPC(const Hash& blockHash, json& ret) {
   Bytes hx = Hex::toBytes(blockHash.hex());
   std::string encodedHexBytes = base64::encode_into<std::string>(hx.begin(), hx.end());
@@ -545,6 +564,11 @@ void Blockchain::incomingBlock(
   const uint64_t syncingToHeight, std::unique_ptr<CometBlock> block, Bytes& appHash,
   std::vector<CometExecTxResult>& txResults, std::vector<CometValidatorUpdate>& validatorUpdates
 ) {
+  std::scoped_lock lock(incomingBlockLockMutex_);
+  while (incomingBlockLock_) {
+    std::this_thread::sleep_for(std::chrono::milliseconds(20));
+  }
+
   try {
     // Update syncing status (don't persist state to disk while syncing (?))
     syncing_ = syncingToHeight > block->height;
diff --git a/src/core/blockchain.h b/src/core/blockchain.h
index 5b8e263b..e959a7de 100644
--- a/src/core/blockchain.h
+++ b/src/core/blockchain.h
@@ -116,6 +116,10 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     std::atomic<bool> started_ = false; ///< Flag to protect the start()/stop() cycle.
 
+    std::mutex incomingBlockLockMutex_; ///< For locking/unlocking block processing.
+
+    std::atomic<bool> incomingBlockLock_ = false; ///< `true` if incomingBlock() is locked.
+
     /**
      * Helper for BDK RPC services, fetches a CometBFT block via CometBFT RPC.
      */
@@ -236,6 +240,24 @@ class Blockchain : public CometListener, public NodeRPCInterface, public Log::Lo
 
     virtual ~Blockchain(); ///< Destructor.
 
+    /**
+     * Lock block processing.
+     * Prevents incomingBlock() Comet callback from executing or being in execution after this call.
+     * Blockchain::state().getHeight() will only return a non-changing value after this call.
+     */
+    void lockBlockProcessing();
+
+    /**
+     * Cancels lockBlockProcessing(), unlocking incomingBlock().
+     */
+    void unlockBlockProcessing();
+
+    /**
+     * Get number of unconfirmed txs in the CometBFT mempool.
+     * @return Integer value returned by RPC num_unconfirmed_txs in result::num_txs, or -1 on error.
+     */
+    int getNumUnconfirmedTxs();
+
     /**
      * Set the size of the GetTx() cache.
      * If you set it to 0, you turn off the cache.
diff --git a/src/core/comet.cpp b/src/core/comet.cpp
index bf6f36bb..356d2d94 100644
--- a/src/core/comet.cpp
+++ b/src/core/comet.cpp
@@ -1294,7 +1294,7 @@ void CometImpl::doStartCometBFT(
     //   that is passed as an arg to setpriv, along with the arguments to forward to cometbft.
     //   however, and that is the point of using setpriv, the resulting cometbft process will receive a SIGTERM
     //   if its parent process (this BDK node process) dies, so that we don't get a dangling cometbft in that case.
-    SLOGDEBUG("Launching cometbft via setpriv --pdeathsig SIGTERM");
+    SLOGTRACE("Launching cometbft via setpriv --pdeathsig SIGTERM");
     exec_path = setpriv_exec_path;
     exec_args = {
       "setpriv",
@@ -1308,7 +1308,7 @@ void CometImpl::doStartCometBFT(
 
   std::string argsString;
   for (const auto& arg : exec_args) { argsString += arg + " "; }
-  SLOGDEBUG("Launching " + exec_path.string() + " with arguments: " + argsString);
+  SLOGTRACE("Launching " + exec_path.string() + " with arguments: " + argsString);
 
   // Launch the process
   auto bpout = std::make_shared<boost::process::ipstream>();
@@ -1320,7 +1320,7 @@ void CometImpl::doStartCometBFT(
       boost::process::std_err > *bperr
   );
   std::string pidStr = std::to_string(process->id());
-  SLOGDEBUG("Launched cometbft with PID: " + pidStr);
+  SLOGTRACE("Launched cometbft with PID: " + pidStr);
 
   // Spawn two detached threads to pump stdout and stderr to bdk.log.
   // They should go away naturally when process is terminated.
@@ -1338,7 +1338,7 @@ void CometImpl::doStartCometBFT(
     }
     // remove trailing \n so that e.g. the node id from cometbft show-node-id is exactly processStdout without a need to trim it.
     if (processStdout) { if (!processStdout->empty()) { processStdout->pop_back(); } }
-    GLOGDEBUG("cometbft stdout stream pump thread finished, cometbft pid = " + pidStr);
+    GLOGTRACE("cometbft stdout stream pump thread finished, cometbft pid = " + pidStr);
     if (processDone) { *processDone = true; } // if actually interested in reading processStderr_ you can just e.g. sleep(1s) first
   });
   std::thread stderr_thread([bperr, pidStr, processStderr]() {
@@ -1348,13 +1348,14 @@ void CometImpl::doStartCometBFT(
         GLOGXTRACE("[cometbft stderr]: " + line);
         *processStderr += line + '\n';
       } else {
-        // If cometbft generates stderr messages, we probably want to see it during regular debugging,
-        // even if we aren't tracing normal cometbft output.
-        GLOGDEBUG("[cometbft stderr]: " + line);
+        // Leaving cometbft output to TRACE is probably optimal.
+        // This makes DEBUG logs far less verbose (when the bug is unrelated to consensus traffic).
+        // NOTE: CometBFT WAL log messages are sent to stderr.
+        GLOGTRACE("[cometbft stderr]: " + line);
       }
     }
     if (processStderr) { if (!processStderr->empty()) { processStderr->pop_back(); } }
-    GLOGDEBUG("cometbft stderr stream pump thread finished, cometbft pid = " + pidStr);
+    GLOGTRACE("cometbft stderr stream pump thread finished, cometbft pid = " + pidStr);
   });
   stdout_thread.detach();
   stderr_thread.detach();
@@ -1363,14 +1364,14 @@ void CometImpl::doStartCometBFT(
 void CometImpl::doStopCometBFT(std::unique_ptr<boost::process::child>& process) {
   // if process is running then we will send SIGTERM to it and if needed SIGKILL
   if (process) {
-    SLOGDEBUG("Terminating CometBFT process");
+    SLOGTRACE("Terminating CometBFT process");
     // terminate the process
     pid_t pid = process->id();
     try {
       process->terminate(); // SIGTERM (graceful termination, equivalent to terminal CTRL+C/SIGINT)
-      SLOGDEBUG("Process with PID " + std::to_string(pid) + " terminated");
+      SLOGTRACE("Process with PID " + std::to_string(pid) + " terminated");
       process->wait();  // Ensure the process is fully terminated
-      SLOGDEBUG("Process with PID " + std::to_string(pid) + " joined");
+      SLOGTRACE("Process with PID " + std::to_string(pid) + " joined");
     } catch (const std::exception& ex) {
       // This is bad, and if it actually happens, we need to be able to do something else here to ensure the process disappears
       //   because we don't want a process using the data directory and using the socket ports.
@@ -1390,12 +1391,12 @@ void CometImpl::doStopCometBFT(std::unique_ptr<boost::process::child>& process)
       }
     }
     process.reset(); // this signals that we are ready to call startCometBFT() again
-    SLOGDEBUG("CometBFT process terminated");
+    SLOGTRACE("CometBFT process terminated");
   }
 }
 
 void CometImpl::workerLoop() {
-  LOGDEBUG("Comet worker thread: started");
+  LOGTRACE("Comet worker thread: started");
   // So basically we take every exception that can be thrown in the inner worker loop and if
   //   we catch one it means the Comet driver goes into TERMINATED state and we record the
   //   error condition as an error message.
@@ -1406,27 +1407,27 @@ void CometImpl::workerLoop() {
   // the state. Note that you *cannot* reenter Comet via Comet::stop() from that callback!
   try {
     workerLoopInner();
-    LOGDEBUG("Comet worker thread: finishing normally (calling cleanup)");
+    LOGTRACE("Comet worker thread: finishing normally (calling cleanup)");
     cleanup();
-    LOGDEBUG("Comet worker thread: finished normally (cleanup done, setting FINISHED state)");
+    LOGTRACE("Comet worker thread: finished normally (cleanup done, setting FINISHED state)");
     setState(CometState::FINISHED); // state transition callback CANNOT reenter Comet::stop()
   } catch (const std::exception& ex) {
     setError("Exception caught in comet worker thread: " + std::string(ex.what()));
-    LOGDEBUG("Comet worker thread: finishing with error (calling cleanup)");
+    LOGTRACE("Comet worker thread: finishing with error (calling cleanup)");
     cleanup();
-    LOGDEBUG("Comet worker thread: finished with error (cleanup done, setting TERMINATED state)");
+    LOGTRACE("Comet worker thread: finished with error (cleanup done, setting TERMINATED state)");
     setState(CometState::TERMINATED); // state transition callback CANNOT reenter Comet::stop()
   }
 }
 
 void CometImpl::workerLoopInner() {
 
-  LOGDEBUG("Comet worker: started");
+  LOGTRACE("Comet worker: started");
 
   // If we are stopping, then quit
   while (!stop_) {
 
-    LOGDEBUG("Comet worker: start loop");
+    LOGTRACE("Comet worker: start loop");
 
     // --------------------------------------------------------------------------------------
     // If this is a continue; and we are restarting the cometbft workerloop, ensure that any
@@ -1437,7 +1438,7 @@ void CometImpl::workerLoopInner() {
 
     cleanup();
 
-    LOGDEBUG("Comet worker: running configuration step");
+    LOGTRACE("Comet worker: running configuration step");
 
     // --------------------------------------------------------------------------------------
     // Run configuration step (writes to the comet/config/* files before running cometbft)
@@ -1481,14 +1482,14 @@ void CometImpl::workerLoopInner() {
 
     const std::string cometUNIXSocketPath = cometPath + "abci.sock";
 
-    LOGDEBUG("Options RootPath: " + options_.getRootPath());
+    LOGINFO("Options RootPath: " + options_.getRootPath());
 
     const json& opt = options_.getCometBFT();
 
     if (opt.is_null()) {
       LOGWARNING("Configuration option cometBFT is null.");
     } else {
-      LOGDEBUG("Configuration option cometBFT: " + opt.dump());
+      LOGTRACE("Configuration option cometBFT: " + opt.dump());
     }
 
     bool hasGenesis = opt.contains(COMET_OPTION_GENESIS_JSON);
@@ -1551,7 +1552,7 @@ void CometImpl::workerLoopInner() {
       throw DynamicException("CometBFT config option p2p::laddr is empty (or isn't a string).");
     } else {
       std::string p2pLaddr = configTomlJSON["p2p"]["laddr"];
-      LOGINFO("CometBFT config option p2p::laddr found: " + p2pLaddr);
+      LOGTRACE("CometBFT config option p2p::laddr found: " + p2pLaddr);
       size_t lastColonPos = p2pLaddr.find_last_of(':');
       if (lastColonPos == std::string::npos || lastColonPos + 1 >= p2pLaddr.size()) {
         setErrorCode(CometError::CONFIG);
@@ -1571,7 +1572,7 @@ void CometImpl::workerLoopInner() {
       throw DynamicException("CometBFT config option rpc::laddr is empty (or isn't a string).");
     } else {
       std::string rpcLaddr = configTomlJSON["rpc"]["laddr"];
-      LOGINFO("CometBFT config option rpc::laddr found: " + rpcLaddr);
+      LOGTRACE("CometBFT config option rpc::laddr found: " + rpcLaddr);
       size_t lastColonPos = rpcLaddr.find_last_of(':');
       if (lastColonPos == std::string::npos || lastColonPos + 1 >= rpcLaddr.size()) {
         setErrorCode(CometError::CONFIG);
@@ -1614,7 +1615,7 @@ void CometImpl::workerLoopInner() {
       }
     }
     genesisJSON["chain_id"] = forceCometBFTChainId;
-    LOGDEBUG("CometBFT chain_id set to '" + forceCometBFTChainId + "' (BDK chainID option).");
+    LOGTRACE("CometBFT chain_id set to '" + forceCometBFTChainId + "' (BDK chainID option).");
 
     // --------------------------------------------------------------------------------------
     // BDK root path must be set up before the Comet worker is started.
@@ -1630,7 +1631,7 @@ void CometImpl::workerLoopInner() {
     //   cometbft init. It will be created with all required options with standard values.
 
     if (!std::filesystem::exists(cometPath)) {
-      LOGDEBUG("Comet worker: creating comet directory");
+      LOGTRACE("Comet worker: creating comet directory");
 
       // run cometbft init cometPath to create the cometbft directory with default configs
       runCometBFT({ "init", "--home=" + cometPath, "-k=secp256k1"});
@@ -1648,7 +1649,7 @@ void CometImpl::workerLoopInner() {
       throw DynamicException("CometBFT home directory is broken: it doesn't have a config/ subdirectory");
     }
 
-    LOGDEBUG("Comet worker: comet directory exists");
+    LOGTRACE("Comet worker: comet directory exists");
 
     // --------------------------------------------------------------------------------------
     // Comet home directory exists; check its configuration is consistent with the current
@@ -1688,7 +1689,7 @@ void CometImpl::workerLoopInner() {
         std::string validatorPubKeyStr = pubKeyObject["value"].get<std::string>();
         std::scoped_lock(this->validatorPubKeyMutex_);
         validatorPubKey_ = base64::decode_into<Bytes>(validatorPubKeyStr);
-        LOGINFO("Validator public key is: " + Hex::fromBytes(validatorPubKey_).get());
+        LOGINFO("Validator public key: " + Hex::fromBytes(validatorPubKey_).get());
       } else {
         setErrorCode(CometError::FATAL);
         throw DynamicException("Cannot open comet privValidatorKey file for writing: " + cometConfigPrivValidatorKeyPath);
@@ -1776,7 +1777,7 @@ void CometImpl::workerLoopInner() {
             setErrorCode(CometError::CONFIG);
             throw DynamicException("Unsupported config JSON value type '" + configType + "' for key: " + logKey);
           }
-          LOGINFO("Setting " + configType + " config: " + logKey + " = " + valueStr);
+          LOGTRACE("Setting " + configType + " config: " + logKey + " = " + valueStr);
         }
       }
     }
@@ -1807,11 +1808,9 @@ void CometImpl::workerLoopInner() {
       throw DynamicException("Failed to close file properly: " + cometConfigTomlPath);
     }
 
-    LOGDEBUG("Comet setting configured");
-
     setState(CometState::CONFIGURED);
 
-    LOGDEBUG("Comet set configured");
+    LOGTRACE("Comet configured");
 
     // --------------------------------------------------------------------------------------
     // Check if quitting
@@ -1820,10 +1819,12 @@ void CometImpl::workerLoopInner() {
     // --------------------------------------------------------------------------------------
     // Run cometbft inspect and check that everything is as expected.
 
+    LOGTRACE("Inspecting comet");
+
     setState(CometState::INSPECTING_COMET);
 
     // Run cometbft show-node-id to figure out what the node ID is
-    LOGDEBUG("Fetching own cometbft node-id...");
+    LOGTRACE("Fetching own cometbft node-id...");
 
     std::string showNodeIdStdout;
     try {
@@ -1838,7 +1839,7 @@ void CometImpl::workerLoopInner() {
       throw DynamicException("Got a cometbft node-id of unexpected size (!= 40 hex chars): [" + showNodeIdStdout + "]");
     }
 
-    LOGDEBUG("Got comet node ID: [" + showNodeIdStdout + "]");
+    LOGINFO("Node ID: " + showNodeIdStdout);
     std::unique_lock<std::mutex> lock(this->nodeIdMutex_);
     this->nodeId_ = showNodeIdStdout;
     this->nodeIdMutex_.unlock();
@@ -1850,7 +1851,7 @@ void CometImpl::workerLoopInner() {
     // Any error thrown will close the running cometbft inspect since it's tracked by process_, just like
     //   cometbft start (regular node) is.
 
-    LOGDEBUG("Starting cometbft inspect");
+    LOGTRACE("Starting cometbft inspect");
 
     // start cometbft inspect
     try {
@@ -1864,7 +1865,7 @@ void CometImpl::workerLoopInner() {
       throw DynamicException("Exception caught when trying to run cometbft inspect: " + std::string(ex.what()));
     }
 
-    LOGDEBUG("Starting RPC connection (inspect)");
+    LOGTRACE("Starting RPC connection (inspect)");
 
     // start RPC connection
     int inspectRpcTries = 50; //5s
@@ -1875,10 +1876,10 @@ void CometImpl::workerLoopInner() {
         inspectRpcSuccess = true;
         break;
       }
-      LOGDEBUG("Retrying RPC connection (inspect): " + std::to_string(inspectRpcTries));
+      LOGTRACE("Retrying RPC connection (inspect): " + std::to_string(inspectRpcTries));
     }
 
-    LOGDEBUG("Done starting RPC connection");
+    LOGTRACE("Done starting RPC connection");
 
     // Check if quitting
     if (stop_) break;
@@ -1889,13 +1890,13 @@ void CometImpl::workerLoopInner() {
     }
 
     json insRes;
-    LOGDEBUG("Making sample RPC call");
+    LOGTRACE("Making sample RPC call");
     if (!rpc_.rpcSyncCall("header", json::object(), insRes)) {
       setErrorCode(CometError::RPC_CALL_FAILED);
       throw DynamicException("ERROR: cometbft inspect RPC header call failed: " + insRes.dump());
     }
 
-    LOGDEBUG("cometbft inspect RPC header call returned OK: "+ insRes.dump());
+    LOGTRACE("cometbft inspect RPC header call returned OK: "+ insRes.dump());
 
     // We got an inspect latest header response; parse it to figure out
     //  lastCometBFTBlockHeight_ and lastCometBFTAppHash_.
@@ -1910,7 +1911,7 @@ void CometImpl::workerLoopInner() {
     const auto& header = insRes["result"]["header"];
     if (header.is_null()) {
       // Header is null, which is valid and indicates an empty block store
-      LOGDEBUG("Header is null; block store is empty.");
+      LOGTRACE("Header is null; block store is empty.");
     } else {
       if ((!header.contains("height") || !header["height"].is_string())) {
         setErrorCode(CometError::RPC_BAD_RESPONSE);
@@ -1923,7 +1924,7 @@ void CometImpl::workerLoopInner() {
       // Got a non-empty header response, so we are past genesis in the cometbft store
       lastCometBFTBlockHeight_ = header["height"].get<std::string>().empty() ? 0 : std::stoull(header["height"].get<std::string>());
       lastCometBFTAppHash_ = header["app_hash"].get<std::string>();
-      LOGDEBUG("Parsed header successfully: Last Block Height = " + std::to_string(lastCometBFTBlockHeight_) + ", Last App Hash = " + lastCometBFTAppHash_);
+      LOGTRACE("Parsed header successfully: Last Block Height = " + std::to_string(lastCometBFTBlockHeight_) + ", Last App Hash = " + lastCometBFTAppHash_);
     }
 
     // FIXME/TODO: if we have at least one block (lastCometBFTBlockHeight_ >= 1) then
@@ -1946,17 +1947,23 @@ void CometImpl::workerLoopInner() {
     // Check if quitting
     if (stop_) break;
 
+    LOGTRACE("Inspect running (breakpoint)");
+
     setState(CometState::INSPECT_RUNNING);
 
     // --------------------------------------------------------------------------------------
     // Finished inspect step.
 
+    LOGTRACE("Finishing inspect");
+
     rpc_.rpcStopConnection();
 
     stopCometBFT();
 
     setState(CometState::INSPECTED_COMET);
 
+    LOGTRACE("Inspected comet");
+
     // --------------------------------------------------------------------------------------
     // Check if quitting
     if (stop_) break;
@@ -1964,6 +1971,8 @@ void CometImpl::workerLoopInner() {
     // --------------------------------------------------------------------------------------
     // Start our cometbft application's ABCI socket server; make sure it is started.
 
+    LOGTRACE("Starting ABCI");
+
     setState(CometState::STARTING_ABCI);
 
     // start the ABCI server
@@ -1984,6 +1993,8 @@ void CometImpl::workerLoopInner() {
 
     setState(CometState::STARTED_ABCI);
 
+    LOGTRACE("Started ABCI");
+
     // --------------------------------------------------------------------------------------
     // Check if quitting
     if (stop_) break;
@@ -1991,6 +2002,8 @@ void CometImpl::workerLoopInner() {
     // --------------------------------------------------------------------------------------
     // Run cometbft start, passing the socket address of our ABCI server as a parameter.
 
+    LOGTRACE("Starting comet");
+
     setState(CometState::STARTING_COMET);
 
     // run cometbft which will connect to our ABCI server
@@ -2021,6 +2034,8 @@ void CometImpl::workerLoopInner() {
 
     setState(CometState::STARTED_COMET);
 
+    LOGTRACE("Started comet");
+
     // --------------------------------------------------------------------------------------
     // Check if quitting
     if (stop_) break;
@@ -2028,6 +2043,8 @@ void CometImpl::workerLoopInner() {
     // --------------------------------------------------------------------------------------
     // Test cometbft: check that the node has started successfully.
 
+    LOGTRACE("Testing comet");
+
     setState(CometState::TESTING_COMET);
 
     // First, wait to receive an ABCI Info callback, which is part of the initial handshake sequence,
@@ -2052,7 +2069,7 @@ void CometImpl::workerLoopInner() {
     if (stop_) break;
 
     // Start RPC connection
-    LOGDEBUG("Will connect to cometbft RPC at port: " + std::to_string(rpcPort_));
+    LOGTRACE("Will connect to cometbft RPC at port: " + std::to_string(rpcPort_));
     int rpcTries = 50; //5s
     bool rpcSuccess = false;
     while (rpcTries-- > 0 && !stop_) {
@@ -2062,7 +2079,7 @@ void CometImpl::workerLoopInner() {
         rpcSuccess = true;
         break;
       }
-      LOGDEBUG("Retrying RPC connection: " + std::to_string(rpcTries));
+      LOGTRACE("Retrying RPC connection: " + std::to_string(rpcTries));
     }
 
     // Check if quitting
@@ -2082,10 +2099,12 @@ void CometImpl::workerLoopInner() {
       setErrorCode(CometError::RPC_CALL_FAILED);
       throw DynamicException("ERROR: cometbft RPC health call failed: " + healthResult.dump());
     }
-    LOGDEBUG("cometbft RPC health call returned OK: " + healthResult.dump());
+    LOGTRACE("cometbft RPC health call returned OK: " + healthResult.dump());
 
     setState(CometState::TESTED_COMET);
 
+    LOGTRACE("Tested comet");
+
     // --------------------------------------------------------------------------------------
     // Check if quitting
     if (stop_) break;
@@ -2096,6 +2115,8 @@ void CometImpl::workerLoopInner() {
 
     setState(CometState::RUNNING);
 
+    LOGTRACE("Comet is running");
+
     // NOTE: If this loop breaks for whatever reason without !stop being true, we will be
     //       in the TERMINATED state, which is there to catch bugs.
     while (!stop_) {
@@ -2193,6 +2214,8 @@ void CometImpl::workerLoopInner() {
       if (stop_) break;
     }
 
+    LOGTRACE("Comet is no longer running");
+
     // --------------------------------------------------------------------------------------
     // If the main loop above exits and this is reached, it is because we are shutting down.
     // Will shut down cometbft, clean up and break loop.
@@ -2207,7 +2230,7 @@ void CometImpl::workerLoopInner() {
     throw DynamicException("Comet worker: exiting (loop end reached); this is an error!");
   }
 
-  LOGDEBUG("Comet worker: exiting (quit loop)");
+  LOGTRACE("Comet worker: exiting (quit loop)");
 }
 
 // CometImpl's ABCIHandler implementation
diff --git a/src/core/state.cpp b/src/core/state.cpp
index 01fce049..be676d42 100644
--- a/src/core/state.cpp
+++ b/src/core/state.cpp
@@ -47,7 +47,7 @@ void State::initChain(
   const std::vector<CometValidatorUpdate>& initialValidators,
   std::string genesisSnapshot
 ) {
-  LOGDEBUG("State::initChain(): Height (BDK, -1) = " + std::to_string(initialHeight));
+  LOGTRACE("State::initChain(): Height (BDK, -1) = " + std::to_string(initialHeight));
 
   // Reset the State (accounts, vmstorage, contracts) and set the genesis
   // starting height and timeMicros (time-in-microseconds) for this State.
@@ -67,20 +67,10 @@ void State::initChain(
 
   // Initial system contract params (ctor params)
   std::vector<std::string> initialValidatorPubKeys;
-  // // Don't need to fetch them from the Options as we get the exact same info via ABCI InitChain params
-  // const auto& validatorsJson = blockchain_.opt().getCometBFT()[COMET_OPTION_GENESIS_JSON]["validators"];
-  // for (const auto& validator : validatorsJson) {
-  //   std::string validatorPubKeyBase64Str = validator["pub_key"]["value"];
-  //   Bytes pubBytes = base64::decode_into<Bytes>(validatorPubKeyBase64Str);
-  //   std::string validatorPubKey = Hex::fromBytes(pubBytes).get();
-  //   LOGDEBUG("Validator PubKey hex string for SystemContract: " + validatorPubKey);
-  //   initialValidatorPubKeys.push_back(validatorPubKey);
-  // }
   for (const auto& validator : initialValidators) {
     // SystemContract takes public keys as hex strings instead of PubKey, unfortunately.
     std::string validatorPubKeyStr = Hex::fromBytes(validator.publicKey).get();
     initialValidatorPubKeys.push_back(validatorPubKeyStr);
-    LOGDEBUG("Validator PubKey hex string for SystemContract: " + validatorPubKeyStr);
   }
 
   uint64_t initialNumSlots = initialValidatorPubKeys.size(); // initial numSlots will simply be the number of validators provided at genesis
@@ -89,7 +79,7 @@ void State::initChain(
   // FIXME/TODO: Must make the SystemContract template undeployable by user code.
   //             Only the BDK node's internal code should be able to deploy it.
 
-  LOGDEBUG("Deploying SystemContract...");
+  LOGTRACE("Deploying SystemContract...");
 
   // Create SystemContract account
   auto& systemContractAcc = *this->accounts_[ProtocolContractAddresses.at("SystemContract")];
@@ -104,8 +94,6 @@ void State::initChain(
       ProtocolContractAddresses.at("SystemContract"), blockchain_.opt().getChainOwner(), blockchain_.opt().getChainID()
     );
 
-  LOGDEBUG("Deployed SystemContract.");
-
   // Set the initial validator set.
   // NOTE: The SystemContract has its own idea of what the validator set is (it stores it
   // in a pair of SafeVector, which is not as useful as the this->validators_ map). However,
@@ -130,10 +118,6 @@ void State::setValidators(const std::vector<CometValidatorUpdate>& newValidatorS
     // Point to the genesis set we are creating.
     currentValidatorSet_ = 0;
   }
-  LOGDEBUG("NEW VALIDATOR SET: " + std::to_string(activationHeight));
-  for (const auto& v : newValidatorSet) {
-    LOGDEBUG("  " + Hex::fromBytes(v.publicKey).get() + " = " + std::to_string(v.power));
-  }
   validatorSets_.emplace_front(
     ValidatorSet(
       activationHeight,
@@ -989,7 +973,6 @@ void State::processBlock(
   SystemContract* systemContractPtr = getSystemContractInternal();
   systemContractPtr->finishBlock(validatorDeltas); // Collect any validator changes accumulated in the singleton system contract...
   if (!validatorDeltas.empty()) {
-    LOGDEBUG("Got validatorDeltas: " + std::to_string(validatorDeltas.size()));
     // Apply validator set deltas
     Bytes validatorDbLog;
     for (const auto& validatorDelta : validatorDeltas) {
@@ -1027,14 +1010,6 @@ void State::processBlock(
     setValidators(newValidatorSet);
   }
 
-  // *********************************************************
-  // FIXME: validatorsets_ is not working
-  // -- print "processblock() height == X"
-  // -- print validatorsets_ size and activation heights of all elements [x,y,z...]
-  // -- print what you are doing to the currentvalidatorset_ and the activation height that it is pointing to
-  // why is getvalidatorset returning 0 and not the new one?
-  // *********************************************************
-
   // Since we have advanced the simulation height, we may need to prune validatorSets_
   // Reverse search for the first validator set that is pending, and for each iteration,
   //   add 1 to the count of old validatorSets_ entries to prune.
@@ -1042,7 +1017,7 @@ void State::processBlock(
   // Example: [pending pending active active active] should prune the last 2.
   int pruneCount = -1; // Start at -1 so the last active is preserved.
   for (auto it = validatorSets_.rbegin(); it != validatorSets_.rend(); ++it) {
-    if (it->getHeight() < height_) {
+    if (it->getHeight() > height_) {
       break; // found first pending validator set, so stop
     }
     ++pruneCount;
diff --git a/src/net/abci/abciserver.cpp b/src/net/abci/abciserver.cpp
index e511b9c5..319c42d5 100644
--- a/src/net/abci/abciserver.cpp
+++ b/src/net/abci/abciserver.cpp
@@ -9,9 +9,9 @@ ABCIServer::ABCIServer(ABCIHandler *handler, const std::string& cometUNIXSocketP
 }
 
 ABCIServer::~ABCIServer() {
-  LOGDEBUG("~ABCIServer(): stopping");
+  LOGXTRACE("~ABCIServer(): stopping");
   stop();
-  LOGDEBUG("~ABCIServer(): stopped");
+  LOGXTRACE("~ABCIServer(): stopped");
 }
 
 const std::string ABCIServer::getSocketPath() {
diff --git a/src/net/http/httpserver.cpp b/src/net/http/httpserver.cpp
index 5548e123..c4a76cd9 100644
--- a/src/net/http/httpserver.cpp
+++ b/src/net/http/httpserver.cpp
@@ -22,12 +22,12 @@ bool HTTPServer::run() {
   std::vector<std::thread> v;
   v.reserve(4 - 1);
   for (int i = 4 - 1; i > 0; i--) v.emplace_back([this]() { this->ioc_.run(); });
-  LOGINFO(std::string("HTTP Server Started at port: ") + std::to_string(port_));
+  LOGTRACE(std::string("HTTP server started at port: ") + std::to_string(port_));
   this->ioc_.run();
 
   // If we get here, it means we got a SIGINT or SIGTERM. Block until all the threads exit
   for (std::thread& t : v) t.join();
-  LOGINFO("HTTP Server Stopped");
+  LOGTRACE("HTTP server stopped");
   return true;
 }
 
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index 7af096d0..7ea2cb0e 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -143,11 +143,13 @@ namespace TBlockchain {
           SDKTestSuite::getOptionsForTest(
             createTestDumpPath("BlockchainValidatorSetTest_" + std::to_string(i)), false, "",
             ports[i].p2p, ports[i].rpc, i, numNodes, ports, numNonValidators, 0,
-            "1s", "1s", httpPorts[i]
+            "500ms", "500ms", httpPorts[i]
           )
         );
       }
 
+      GLOGDEBUG("TEST: Starting Blockchain instances one by one; this will take a while...");
+
       std::vector<std::unique_ptr<Blockchain>> blockchains;
       for (int i = 0; i < numNodes; ++i) {
         blockchains.emplace_back(std::make_unique<Blockchain>(options[i], std::to_string(i)));
@@ -157,6 +159,8 @@ namespace TBlockchain {
         blockchains.back()->start();
       }
 
+      GLOGDEBUG("TEST: Started all Blockchain instances");
+
       std::vector<CometValidatorUpdate> origValidatorSet;
       uint64_t origValidatorSetHeight = 0;
 
@@ -217,6 +221,9 @@ namespace TBlockchain {
       GLOGDEBUG("Test: start first delegations");
 
       // Nodes #0 .. #4 delegate 5, 4, 3, 2, 1 for themselves, respectively
+      // This results in nodes #0, #1, #2, #3 elected, since there are 4 validator slots (set on genesis)
+      // These are the same 4 validator keys that were set on genesis
+      // Node #4 only gets 1 vote (from self) so it loses the election to the other 4 nodes
       for (int i = 0; i < 5; ++i) {
         blockchainCallCpp(
           *blockchains[0], // use node 0 to call (could be any node)
@@ -229,41 +236,256 @@ namespace TBlockchain {
         );
       }
 
-      // After all transactions went through, the validator set with 4 slots should be completely unchanged.
+      // After all transactions went through, the validator set with 4 slots should contain the same validators,
+      //   but possibly (and probably) in a different order, because the voting power has changed from 10,10,10,10
+      //   to 5 eth, 4 eth, 3 th and 2 eth. The original sorting order was arbitrarily determined by the genesis set.
       // NOTE: just wait a while and then read the blockchain's validator set, as the blockchain's validator set
       //       is modified (or not) by the system contract when it receives delegate calls.
       //       it could be that the system contract changed but the blockchain validator set wasn't notified, but
       //       this check won't catch that kind of bug here.
       GLOGDEBUG("Test: end first delegations");
-      std::this_thread::sleep_for(std::chrono::milliseconds(10'000)); // we actually need to wait for H+2 activation!
-      GLOGDEBUG("Test: checking validator set did not change");
-      for (int i = 0; i < 1; ++i) { // FIXME: to 5
+      std::this_thread::sleep_for(std::chrono::milliseconds(5'000)); // we actually need to wait for H+2 activation!
+      GLOGDEBUG("Test: checking that the exact same validator keys are still elected (order and voting power is OK to change)");
+      for (int i = 0; i < 5; ++i) {
         std::vector<CometValidatorUpdate> validatorSet;
         uint64_t validatorSetHeight = 0;
         blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
         REQUIRE(origValidatorSet.size() == validatorSet.size()); // == numValidators, already asserted above
-        GLOGDEBUG("COMPARE VALIDATOR SET: old vs new activation height: " + std::to_string(origValidatorSetHeight) + " --> " + std::to_string(validatorSetHeight));
+        std::set<PubKey> keysToMatch;
+        for (int j = 0; j < validatorSet.size(); ++j) {
+          keysToMatch.insert(PubKey(origValidatorSet[j].publicKey));
+        }
+        for (int j = 0; j < validatorSet.size(); ++j) {
+          keysToMatch.erase(PubKey(validatorSet[j].publicKey));
+        }
+        REQUIRE(keysToMatch.empty()); // orig and current were same size and had the exact same keys (in whatever order, doesn't matter)
+        REQUIRE(origValidatorSetHeight != validatorSetHeight); // they should be different, obviously (orig is 0, current is > 0)
+      }
+
+      // Delegates 10 tokens for node #5
+      GLOGDEBUG("Test: delegating to node 5, should push node 3 out and be (5, 0, 1, 2)");
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[5],
+        systemContractAddr, // to: SystemContract
+        0, // delegate does not send native tokens
+        &SystemContract::delegate, // calls SystemContract::delegate() to stake tokens on specific validator
+        Hex::fromBytes(nodeAccs[5].pubKey).get(), // self delegation (node is "registering" itself -- first delegation must be from/to self)
+        uint256_t("1000000000000000000") * 10 // node 5 delegates 10 eth to itself
+      );
+
+      // After tx goes through, resulting validator set should be #5, #0, #1, #2 for the 4 slots (node #3 gets pushed out)
+      std::this_thread::sleep_for(std::chrono::milliseconds(5'000)); // we actually need to wait for H+2 activation!
+      GLOGDEBUG("Test: checking node 5 pushes node 3 out (5, 0, 1, 2)");
+      for (int i = 0; i < 5; ++i) {
+        std::vector<CometValidatorUpdate> validatorSet;
+        uint64_t validatorSetHeight = 0;
+        blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
+        REQUIRE(validatorSet.size() == numValidators); // still 4 slots
+        std::set<PubKey> keysToMatch;
+        keysToMatch.insert(nodeAccs[5].pubKey);
+        keysToMatch.insert(nodeAccs[0].pubKey);
+        keysToMatch.insert(nodeAccs[1].pubKey);
+        keysToMatch.insert(nodeAccs[2].pubKey);
         for (int j = 0; j < validatorSet.size(); ++j) {
-          GLOGDEBUG("  ORIG: " + Hex::fromBytes(origValidatorSet[j].publicKey).get() + " = " + std::to_string(origValidatorSet[j].power));
-          GLOGDEBUG("  NEW : " + Hex::fromBytes(validatorSet[j].publicKey).get() + " = " + std::to_string(validatorSet[j].power));
-          REQUIRE(origValidatorSet[j].publicKey == validatorSet[j].publicKey);
-          REQUIRE(origValidatorSet[j].power == validatorSet[j].power); // FIXME: this should FAIL, the power should have changed.
+          keysToMatch.erase(PubKey(validatorSet[j].publicKey));
         }
-        REQUIRE(origValidatorSetHeight == validatorSetHeight); // both should be 0 (genesis)
+        REQUIRE(keysToMatch.empty());
       }
 
-      // TODO:
-      // Chain owner delegates 10 tokens for node #5
-      // After tx goes through, resulting validator set should be #5, #0, #1, #2 for the 4 slots.
+      // Node #3 is no longer elected, so it cannot vote for slots
+      GLOGDEBUG("Test: node #3 votes for 4 slots (vote should not be accepted since node #3 is no longer elected)");
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[3],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[3].pubKey).get(), // caller key
+        4UL // or static_cast<uint64_t>(4)
+      );
+
+      // Validators #5, #0 vote to change number of slots to 5, 5
+      GLOGDEBUG("Test: nodes #5 and #0 vote for 5 slots");
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[5],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[5].pubKey).get(), // caller key
+        5UL
+      );
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[0],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[0].pubKey).get(), // caller key
+        5UL
+      );
+
+      // If node #3's vote is considered (which is an error), it will change to 4 after all three votes go through since there are 4 slots and 3 votes (>2/3)
+      // So we expect the number of slots to not change.
+      std::this_thread::sleep_for(std::chrono::milliseconds(5'000)); // we actually need to wait for H+2 activation!
+      GLOGDEBUG("Test: checking number of slots is unchanged across the entire network");
+      for (int i = 0; i < 5; ++i) {
+        std::vector<CometValidatorUpdate> validatorSet;
+        uint64_t validatorSetHeight = 0;
+        blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
+        REQUIRE(validatorSet.size() == numValidators); // still 4 slots
+      }
+
+      // Node #1 vote to change the number of slots to 6
+      GLOGDEBUG("Test: nodes #1 votes for 6 slots");
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[1],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[1].pubKey).get(), // caller key
+        6UL // valid votes should now be 5, 5, 6 (75% of elected validators want to change upwards), and 5 is the greatest increase that 2/3 agree with
+      );
 
-      // TODO:
-      // Validators #5, #0, and #1 vote to change the number of slots: 5, 5, 6
       // After tx goes through, number of slots should change to 5, making the validator set be: #5, #0, #1, #2, #3
+      std::this_thread::sleep_for(std::chrono::milliseconds(5'000)); // we actually need to wait for H+2 activation!
+      GLOGDEBUG("Test: checking number of slots changed to 5 and elected validators are 5, 0, 1, 2, 3 (in order)");
+      for (int i = 0; i < 5; ++i) {
+        std::vector<CometValidatorUpdate> validatorSet;
+        uint64_t validatorSetHeight = 0;
+        blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
+        REQUIRE(validatorSet.size() == 5); // slot increase
+        REQUIRE(PubKey(validatorSet[0].publicKey) == nodeAccs[5].pubKey);
+        REQUIRE(PubKey(validatorSet[1].publicKey) == nodeAccs[0].pubKey);
+        REQUIRE(PubKey(validatorSet[2].publicKey) == nodeAccs[1].pubKey);
+        REQUIRE(PubKey(validatorSet[3].publicKey) == nodeAccs[2].pubKey);
+        REQUIRE(PubKey(validatorSet[4].publicKey) == nodeAccs[3].pubKey);
+      }
 
-      // TODO:
       // Validator #1 gets fully undelegated (-4 tokens).
+      GLOGDEBUG("Test: fully undelegating from node 1, should push node 1 out and be (5, 0, 2, 3, 4)");
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[1],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::undelegate, // calls SystemContract::undelegate() to unvote validator
+        Hex::fromBytes(nodeAccs[1].pubKey).get(), // self undelegation
+        uint256_t("1000000000000000000") * 4 // node 1 undelegates 4 eth from itself, now 0 votes total, node 4 gets elected in its place
+      );
+
       // After tx goes through, validator set should be #5, #0, #2, #3, #4
+      std::this_thread::sleep_for(std::chrono::milliseconds(5'000)); // we actually need to wait for H+2 activation!
+      GLOGDEBUG("Test: checking elected validators are 5, 0, 2, 3, 4 (in order)");
+      for (int i = 0; i < 5; ++i) {
+        std::vector<CometValidatorUpdate> validatorSet;
+        uint64_t validatorSetHeight = 0;
+        blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
+        REQUIRE(validatorSet.size() == 5); // same number of slots and we still have at least 5 validators to fill in these 5 slots
+                                           // (if node 4 had also fully undelegated, we'd see only 4 validators elected for the 5 slots)
+        REQUIRE(PubKey(validatorSet[0].publicKey) == nodeAccs[5].pubKey);
+        REQUIRE(PubKey(validatorSet[1].publicKey) == nodeAccs[0].pubKey);
+        REQUIRE(PubKey(validatorSet[2].publicKey) == nodeAccs[2].pubKey);
+        REQUIRE(PubKey(validatorSet[3].publicKey) == nodeAccs[3].pubKey);
+        REQUIRE(PubKey(validatorSet[4].publicKey) == nodeAccs[4].pubKey);
+      }
+
+      // for the next test, we need to lock block finalization (incomingBlock() on the entire network, which will essentially stall
+      // block proposal and thus mempool tx selection for blocks). we need this because we need all CPP calls below to go in the same block.
+      GLOGDEBUG("TEST: locking block processing across entire network");
+      for (int i = 0; i < numNodes; ++i) {
+        blockchains[i]->lockBlockProcessing();
+      }
 
+      // set decrease numslot votes: 1, 1, 2, 3, 4 by sending 5 voteSlots txs
+      GLOGDEBUG("Test: elected validators will vote for numslots decrease: 1,1,2,3,4 (doesn't matter which validator votes what)");
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[0],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[0].pubKey).get(), // caller key
+        1UL
+      );
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[2],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[2].pubKey).get(), // caller key
+        1UL
+      );
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[3],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[3].pubKey).get(), // caller key
+        2UL
+      );
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[4],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[4].pubKey).get(), // caller key
+        3UL
+      );
+      blockchainCallCpp(
+        *blockchains[0], // use node 0 to call (could be any node)
+        nodeAccs[5],
+        systemContractAddr, // to: SystemContract
+        0,
+        &SystemContract::voteSlots,
+        Hex::fromBytes(nodeAccs[5].pubKey).get(), // caller key
+        4UL
+      );
+
+      // wait a little bit to ensure all 5 voteSlots txs above can enter all mempools
+      GLOGDEBUG("TEST: waiting for all 5 txs to enter mempools across the entire network");
+      int decvoteTxsTimeout = 11;
+      while (--decvoteTxsTimeout > 0) { // 10 iterations = 10s
+        int ok = 0;
+        for (int i = 0; i < numNodes; ++i) {
+          if (blockchains[i]->getNumUnconfirmedTxs() >= 5) {
+            ++ok;
+          }
+        }
+        if (ok == numNodes) {
+          break;
+        }
+        std::this_thread::sleep_for(std::chrono::milliseconds(1'000));
+      }
+      REQUIRE(decvoteTxsTimeout > 0);
+
+      // now that the txs are in all mempools, unlock block processing so all the 5 txs can go to the same prepareproposal (block)
+      // by whatever validator gets picked as the next proposer.
+      // this works because holding up the FinalizeBlock ABCI callback across the entire network will prevent the next proposer
+      // from advancing to the PrepareProposal step, which includes flushing the mempool into the next block (since it's just a
+      // tiny amount of tx data and blocks tolerate megabytes of data, it's 100% odds that all of them go in the next block).
+      GLOGDEBUG("TEST: unlocking block processing across entire network");
+      for (int i = 0; i < numNodes; ++i) {
+        blockchains[i]->unlockBlockProcessing();
+      }
+
+      // should reduce to 3, not 4, since 2/3 threshold is met by 3 (the vote on 4 is skipped)
+      std::this_thread::sleep_for(std::chrono::milliseconds(5'000)); // we actually need to wait for H+2 activation!
+      GLOGDEBUG("Test: checking elected validators are 5, 0, 2 (in order)");
+      for (int i = 0; i < 5; ++i) {
+        std::vector<CometValidatorUpdate> validatorSet;
+        uint64_t validatorSetHeight = 0;
+        blockchains[i]->state().getValidatorSet(validatorSet, validatorSetHeight);
+        REQUIRE(validatorSet.size() == 3); // decrease slots vote should have evaluated to 3
+        REQUIRE(PubKey(validatorSet[0].publicKey) == nodeAccs[5].pubKey);
+        REQUIRE(PubKey(validatorSet[1].publicKey) == nodeAccs[0].pubKey);
+        REQUIRE(PubKey(validatorSet[2].publicKey) == nodeAccs[2].pubKey);
+      }
       GLOGDEBUG("TEST: Validator set test finished.");
     }
 /*
diff --git a/tests/sdktestsuite.hpp b/tests/sdktestsuite.hpp
index 11bcc46b..20d33799 100644
--- a/tests/sdktestsuite.hpp
+++ b/tests/sdktestsuite.hpp
@@ -176,6 +176,9 @@ class SDKTestSuite : public Blockchain {
       if (std::filesystem::exists(testDumpPath)) {
         std::filesystem::remove_all(testDumpPath);
       }
+      if (std::filesystem::exists(testDumpPath)) {
+        throw DynamicException("Failed to delete old test dump path: " + testDumpPath);
+      }
       std::filesystem::create_directories(testDumpPath);
       GLOGDEBUG("Test dump path: " + testDumpPath);
       return testDumpPath;

From b477e3101107548a76d8c8542f8cc7cb944d2dd4 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Fri, 21 Feb 2025 16:18:40 -0300
Subject: [PATCH 13/15] cleanups

---
 src/contract/templates/systemcontract.cpp | 21 +--------------------
 src/contract/templates/systemcontract.h   |  3 +--
 2 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index 50b0ec0c..a301f660 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -36,7 +36,7 @@ struct ValidatorVotes {
   }
 };
 
-bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive) {
+void SystemContract::recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive) {
   // check that the current validator votes + the current delegation delta + the new delta won't
   // end outside of uint64_t range. if OK, save as intermediary int256_t at delegationDeltas_.
   int256_t checker = 0;
@@ -72,7 +72,6 @@ bool SystemContract::recordDelegationDelta(const PubKey& validator, const uint64
   } else {
     delegationDeltas_[validator] -= delta;
   }
-  return true;
 }
 
 SystemContract::SystemContract(
@@ -218,15 +217,6 @@ void SystemContract::delegate(const std::string& validatorPubKey, const uint256_
     }
   }
   delegations_[caller][validator] += amount64;
-  // This is not needed as we imposed a total ordering based on public key binary comparison
-  // // Loop avoiding collision in delegation amount (which we do not support)
-  // while (!recordDelegationDelta(validator, amount64, true)) {
-  //  if (amount64 == 1) {
-  //    throw DynamicException("Cannot delegate this amount (try a larger amount)");
-  //  }
-  //  --amount64;
-  //  --delegations_[caller][validator];
-  //}
   recordDelegationDelta(validator, amount64, true);
 }
 
@@ -259,15 +249,6 @@ void SystemContract::undelegate(const std::string& validatorPubKey, const uint25
   if (effectiveAmount > 0) {
     stakes_[caller] += effectiveAmount;
   }
-  // This is not needed as we imposed a total ordering based on public key binary comparison
-  // // Loop avoiding collision in delegation amount (which we do not support)
-  // while (!recordDelegationDelta(validator, amount64, false)) {
-  //   if (amount64 == 1) {
-  //     throw DynamicException("Cannot undelegate this amount (try a larger amount)");
-  //   }
-  //   --amount64;
-  //   ++delegations_[caller][validator];
-  // }
   recordDelegationDelta(validator, amount64, false);
 }
 
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index dc2f9733..2bace132 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -73,7 +73,7 @@ class SystemContract : public DynamicContract {
 
     static PubKey pubKeyFromString(const std::string& pubKeyStr);
 
-    bool recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive);
+    void recordDelegationDelta(const PubKey& validator, const uint64_t& delta, const bool& positive);
 
     SafeUint64_t numSlots_; /// The active number of validator slots.
     SafeUint64_t maxSlots_; /// The maximum value for numSlots_.
@@ -177,7 +177,6 @@ class SystemContract : public DynamicContract {
     /**
      * An elected validator sets its vote for the number of validator slots.
      * The tx sender (caller) must be the account for validatorPubKey.
-     * TODO: If we keep an account-to-pubkey reverse mapping, we don't need to take validatorPubKey as a param.
      * @param validatorPubKey 33-byte Secp256k1 validador public key that is voting for a number of slots (must match caller account).
      * @param slots Chosen target number of validator slots to vote for.
      */

From bb633e21fefc24b8f527cb27f4b54b940540cdd7 Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Fri, 21 Feb 2025 16:37:22 -0300
Subject: [PATCH 14/15] add halley branch to CI

---
 .github/workflows/c-cpp.yml               | 2 ++
 src/contract/templates/systemcontract.cpp | 2 ++
 src/contract/templates/systemcontract.h   | 8 +-------
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/.github/workflows/c-cpp.yml b/.github/workflows/c-cpp.yml
index 3fb8fd31..06d6fbf7 100644
--- a/.github/workflows/c-cpp.yml
+++ b/.github/workflows/c-cpp.yml
@@ -5,10 +5,12 @@ on:
     branches:
       - main
       - development
+      - halley
   pull_request:
     branches:
       - main
       - development
+      - halley
 
 jobs:
   build_test_and_analyse:
diff --git a/src/contract/templates/systemcontract.cpp b/src/contract/templates/systemcontract.cpp
index a301f660..6011598b 100644
--- a/src/contract/templates/systemcontract.cpp
+++ b/src/contract/templates/systemcontract.cpp
@@ -11,6 +11,8 @@ See the LICENSE.txt file in the project root for more information.
 
 #include <set>
 
+// REVIEW: Should we make a PubKey an actual ABI type instead? (that is, a
+// type that can be used as a parameter type for registerd CPP contract methods)
 PubKey SystemContract::pubKeyFromString(const std::string& pubKeyStr) {
   Bytes pubKeyBytes = Hex::toBytes(pubKeyStr);
   if (pubKeyBytes.size() != 33) {
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index 2bace132..90443e9c 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -27,13 +27,7 @@ See the LICENSE.txt file in the project root for more information.
  * only a problem in the current overflow protection code if intermediary calculations exceed
  * int256_t, so it's never a problem).
  *
- * TODO:
- * - Prevent SystemContract from being instantiated by incoming transactions.
- * - Actually track pending vs. active validator sets in Blockchain.
- *   (System contract tracks only the latest elected version and is correct).
- * - Contract class is fully written and compiles.
- * - Write unit test.
- * - PR branch.
+ * TODO: Prevent SystemContract from being instantiated by incoming transactions.
  */
 class SystemContract : public DynamicContract {
   private:

From 4d9e2653567d5b2f792e636baf3c66fce573c9dc Mon Sep 17 00:00:00 2001
From: Fabiana Cecin <fcecin@gmail.com>
Date: Fri, 21 Feb 2025 20:02:12 -0300
Subject: [PATCH 15/15] Prevent SystemContract from being instantiated by
 incoming transactions.

---
 .github/workflows/c-cpp.yml             |  2 --
 src/contract/customcontracts.h          | 21 +++++++++++++++++++--
 src/contract/templates/systemcontract.h |  2 --
 src/core/state.cpp                      |  7 ++-----
 src/core/state.h                        | 10 ++++++++--
 tests/integration/core/blockchain.cpp   | 23 +++++++++++++++++++++--
 6 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/.github/workflows/c-cpp.yml b/.github/workflows/c-cpp.yml
index 06d6fbf7..3fb8fd31 100644
--- a/.github/workflows/c-cpp.yml
+++ b/.github/workflows/c-cpp.yml
@@ -5,12 +5,10 @@ on:
     branches:
       - main
       - development
-      - halley
   pull_request:
     branches:
       - main
       - development
-      - halley
 
 jobs:
   build_test_and_analyse:
diff --git a/src/contract/customcontracts.h b/src/contract/customcontracts.h
index 29a028fa..412fb8db 100644
--- a/src/contract/customcontracts.h
+++ b/src/contract/customcontracts.h
@@ -5,6 +5,9 @@ This software is distributed under the MIT License.
 See the LICENSE.txt file in the project root for more information.
 */
 
+#ifndef CUSTOMCONTRACTS_H
+#define CUSTOMCONTRACTS_H
+
 #include "templates/erc20.h"
 #include "templates/erc20wrapper.h"
 #include "templates/nativewrapper.h"
@@ -30,14 +33,28 @@ See the LICENSE.txt file in the project root for more information.
 /// Typedef for the blockchain's registered contracts in TESTNET mode.
 using ContractTypes = std::tuple<
   ERC20, NativeWrapper, DEXV2Pair, DEXV2Factory, DEXV2Router02, ERC721, ERC721URIStorage,
-  Ownable, Pebble, SystemContract
+  Ownable, Pebble
 >;
 #else
 /// Typedef for the blockchain's registered contracts in normal mode.
 using ContractTypes = std::tuple<
   ERC20, ERC20Wrapper, NativeWrapper, SimpleContract, DEXV2Pair, DEXV2Factory,
   DEXV2Router02, ERC721, ThrowTestA, ThrowTestB, ThrowTestC, ERC721Test, TestThrowVars,
-  RandomnessTest, SnailTracer, SnailTracerOptimized, Pebble, SystemContract
+  RandomnessTest, SnailTracer, SnailTracerOptimized, Pebble
 >;
 #endif
 
+/// Typedef for all protocol contracts that have state to persist.
+using StatefulProtocolContractTypes = std::tuple<
+  SystemContract
+>;
+
+/// Typedef for all contracts (protocol or not) with state to persist.
+using PersistedContractTypes = decltype(
+  std::tuple_cat(
+    std::declval<ContractTypes>(),
+    std::declval<StatefulProtocolContractTypes>()
+  )
+);
+
+#endif // CUSTOMCONTRACTS_H
\ No newline at end of file
diff --git a/src/contract/templates/systemcontract.h b/src/contract/templates/systemcontract.h
index 90443e9c..1e94ee16 100644
--- a/src/contract/templates/systemcontract.h
+++ b/src/contract/templates/systemcontract.h
@@ -26,8 +26,6 @@ See the LICENSE.txt file in the project root for more information.
  * generate that many tokens (total) but we should do our best to protect against this (it's
  * only a problem in the current overflow protection code if intermediary calculations exceed
  * int256_t, so it's never a problem).
- *
- * TODO: Prevent SystemContract from being instantiated by incoming transactions.
  */
 class SystemContract : public DynamicContract {
   private:
diff --git a/src/core/state.cpp b/src/core/state.cpp
index be676d42..ffc81e6f 100644
--- a/src/core/state.cpp
+++ b/src/core/state.cpp
@@ -76,9 +76,6 @@ void State::initChain(
   uint64_t initialNumSlots = initialValidatorPubKeys.size(); // initial numSlots will simply be the number of validators provided at genesis
   uint64_t maxSlots = 1'000; // Set this to the global maximum numslots for BDK chains
 
-  // FIXME/TODO: Must make the SystemContract template undeployable by user code.
-  //             Only the BDK node's internal code should be able to deploy it.
-
   LOGTRACE("Deploying SystemContract...");
 
   // Create SystemContract account
@@ -340,7 +337,7 @@ void State::saveSnapshot(const std::string& where) {
     // REVIEW: Snapshotting should be done by a dedicated snapshotter node, and it may
     // make sense to add parallelization here (if the snapshotter will be using a dedicated
     // machine, for example). In that case, the number of snapshotter threads should be
-    // a command-lin, process-wide option e.g. --snapshot_threads <N>.
+    // a command-line, process-wide option e.g. --snapshot_threads <N>.
     out.putBatch(baseContractPtr->dump());
   }
 
@@ -461,7 +458,7 @@ void State::loadSnapshot(const std::string& where, bool genesisSnapshot) {
   uint64_t ctCount = 0;
   for (const DBEntry& contract : in.getBatch(DBPrefix::contractManager)) {
     Address address(contract.key);
-    if (!cmPtr->loadFromDB<ContractTypes>(contract, address, in)) {
+    if (!cmPtr->loadFromDB<PersistedContractTypes>(contract, address, in)) {
       throw DynamicException("Read corrupt snapshot; unknown contract: " + StrConv::bytesToString(contract.value));
     }
     ++ctCount;
diff --git a/src/core/state.h b/src/core/state.h
index a063f757..c5118452 100644
--- a/src/core/state.h
+++ b/src/core/state.h
@@ -346,10 +346,16 @@ class State : public Log::LogicalLocationProvider {
      */
     int64_t estimateGas(EncodedMessageVariant msg);
 
-    /// Get a list of the C++ contract addresses and names.
+    /**
+     * Get a list of the C++ contract addresses and names.
+     * NOTE: TESTING ONLY, as this iterates over ALL contracts_.
+     */
     std::vector<std::pair<std::string, Address>> getCppContracts() const;
 
-    /// Get a list of Addresss which are EVM contracts.
+    /**
+     * Get a list of Addresss which are EVM contracts.
+     * NOTE: TESTING ONLY, as this iterates over ALL accounts_.
+     */
     std::vector<Address> getEvmContracts() const;
 
     /**
diff --git a/tests/integration/core/blockchain.cpp b/tests/integration/core/blockchain.cpp
index 7ea2cb0e..6c6442fb 100644
--- a/tests/integration/core/blockchain.cpp
+++ b/tests/integration/core/blockchain.cpp
@@ -82,6 +82,8 @@ void blockchainSendNativeTokens(
 ) {
   Bytes txData;
   Gas gas(1'000'000'000);
+  // TODO/REVIEW: is this "estimateGas" reverting or does it have the same problem
+  //              as blockchainCallCpp() above?
   const uint64_t gasUsed = 10'000 + blockchain.state().estimateGas(
     EncodedCallMessage(fromAccount.address, toAddress, gas, value, txData)
   );
@@ -126,6 +128,24 @@ namespace TBlockchain {
   TEST_CASE("Blockchain Class", "[integration][core][blockchain]") {
     std::string testDumpPath = Utils::getTestDumpPath();
 
+    // Check that we can't deploy SystemContract twice in Blockchain
+    // (SDKTestSuite is a Blockchain subclass)
+    SECTION("BlockchainSystemContractSingletonTest") {
+      SDKTestSuite sdk = SDKTestSuite::createNewEnvironment("BlockchainSystemContractSingletonTest");
+      // The constructor arguments can be whatever that won't make the actual class ctor itself throw.
+      std::vector<std::string> initialValidatorPubKeys = {"030000000000000000000000000000000000000000000000000000000000000000"};
+      uint64_t initialNumSlots = 12;
+      uint64_t maxSlots = 34;
+      // If `SystemContract` is added to the `ContractTypes` tuple (in `customcontracts.h`) then this does NOT throw.
+      // Which means that not including `SystemContract` in `ContractTypes` is sufficient to render it undeployable by transactions.
+      GLOGDEBUG("TEST: trying to spawn a SystemContract");
+      REQUIRE_THROWS(
+        sdk.deployContract<SystemContract>(initialValidatorPubKeys, initialNumSlots, maxSlots)
+      );
+      GLOGDEBUG("TEST: finished trying to spawn a SystemContract");
+    }
+
+    // SystemContract integration test with Blockchain
     SECTION("BlockchainValidatorSetTest") {
       const int numNodes = 6;
       const int numNonValidators = 2;
@@ -488,7 +508,7 @@ namespace TBlockchain {
       }
       GLOGDEBUG("TEST: Validator set test finished.");
     }
-/*
+
     SECTION("BlockchainBootTest") {
       std::string testDumpPath = createTestDumpPath("BlockchainBootTest");
 
@@ -923,7 +943,6 @@ namespace TBlockchain {
 
       GLOGDEBUG("TEST: done");
     }
-*/
   }
 }