Merge #4827

4827: Revert keeping zero bids in the auction state r=fizyk20 a=fizyk20

This reverts the changes introduced in #4719, as they are no longer needed since #4728 got merged and they were the reason behind the recent issues in tests.

Since the logic introduced in #4719 expected zero bids to exist until the unbonds are processed, and 1.5 removed them upon bid withdrawal / undelegation, this caused issues during the upgrade: 2.0 was attempting to remove zero bids which had already been removed, and failing with a `ValidatorNotFound` error. Thus, the simplest fix was to revert those changes.

This PR also adds a few debug messages that proved to be useful in figuring out this issue.

Closes #4808, #4811, #4812

Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io>

execution_engine_testing/tests/src/test/regression/regression_20210831.rs

@@ -439,10 +439,8 @@ fn regression_20210831_should_fail_to_activate_bid() {
     builder.exec(withdraw_bid_request).expect_success().commit();
 
     let bids = builder.get_bids();
-    let bid = bids
-        .validator_bid(&DEFAULT_ACCOUNT_PUBLIC_KEY)
-        .expect("should have zero bid");
-    assert!(bid.staked_amount().is_zero());
+    let bid = bids.validator_bid(&DEFAULT_ACCOUNT_PUBLIC_KEY);
+    assert!(bid.is_none());
 
     let sender = *ACCOUNT_2_ADDR;
     let activate_bid_args = runtime_args! {

execution_engine_testing/tests/src/test/system_contracts/auction/bids.rs

@@ -2054,11 +2054,7 @@ fn should_undelegate_delegators_when_validator_unbonds() {
         .expect_success();
 
     let bids_after = builder.get_bids();
-    assert!(bids_after
-        .validator_bid(&VALIDATOR_1)
-        .expect("should still have zero bid")
-        .staked_amount()
-        .is_zero());
+    assert!(bids_after.validator_bid(&VALIDATOR_1).is_none());
 
     let unbonding_purses_after: UnbondingPurses = builder.get_unbonds();
     assert_ne!(unbonding_purses_after, unbonding_purses_before);
@@ -2261,11 +2257,7 @@ fn should_undelegate_delegators_when_validator_fully_unbonds() {
         .expect_success();
 
     let bids_after = builder.get_bids();
-    assert!(bids_after
-        .validator_bid(&VALIDATOR_1)
-        .expect("should still have zero bid")
-        .staked_amount()
-        .is_zero());
+    assert!(bids_after.validator_bid(&VALIDATOR_1).is_none());
 
     let unbonding_purses_before: UnbondingPurses = builder.get_unbonds();
 
@@ -2324,9 +2316,6 @@ fn should_undelegate_delegators_when_validator_fully_unbonds() {
         timestamp_millis += TIMESTAMP_MILLIS_INCREMENT;
     }
 
-    let bids_after_2 = builder.get_bids();
-    assert!(bids_after_2.validator_bid(&VALIDATOR_1).is_none());
-
     let validator_1_balance_after = builder.get_purse_balance(validator_1.main_purse());
     let delegator_1_balance_after = builder.get_purse_balance(delegator_1.main_purse());
     let delegator_2_balance_after = builder.get_purse_balance(delegator_2.main_purse());
@@ -4312,9 +4301,6 @@ fn should_enforce_max_delegators_per_validator_cap() {
 
     assert_eq!(current_delegator_count, 1);
 
-    // Make the undelegation go through and remove the bid completely.
-    builder.advance_eras_by(DEFAULT_UNBONDING_DELAY + 1);
-
     let delegation_request_3 = ExecuteRequestBuilder::standard(
         *DELEGATOR_1_ADDR,
         CONTRACT_DELEGATE,

execution_engine_testing/tests/src/test/system_contracts/auction/distribute.rs

@@ -378,22 +378,27 @@ fn should_distribute_delegation_rate_zero() {
             VALIDATOR_1.clone(),
             validator_1_actual_payout + U512::from(VALIDATOR_1_STAKE),
         );
-        get_validator_bid(&mut builder, VALIDATOR_1.clone())
-            .expect("should still have zero bid")
-            .staked_amount()
+        assert!(get_validator_bid(&mut builder, VALIDATOR_1.clone()).is_none());
+        U512::zero()
     };
     assert_eq!(validator_1_balance, U512::zero());
 
-    let delegator_1_balance =
-        get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone())
-            .expect("validator withdrawing full stake also removes delegator 1 reinvested funds")
-            .staked_amount();
+    let delegator_1_balance = {
+        assert!(
+            get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone()).is_none(),
+            "validator withdrawing full stake also removes delegator 1 reinvested funds"
+        );
+        U512::zero()
+    };
     assert_eq!(delegator_1_balance, U512::zero());
 
-    let delegator_2_balance =
-        get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_2.clone())
-            .expect("validator withdrawing full stake also removes delegator 1 reinvested funds")
-            .staked_amount();
+    let delegator_2_balance = {
+        assert!(
+            get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_2.clone()).is_none(),
+            "validator withdrawing full stake also removes delegator 1 reinvested funds"
+        );
+        U512::zero()
+    };
     assert!(delegator_2_balance.is_zero());
 
     let era_info = get_era_info(&mut builder);
@@ -651,10 +656,7 @@ fn should_withdraw_bids_after_distribute() {
             undelegate_amount,
         );
         assert!(
-            get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone())
-                .expect("should still have zero bid")
-                .staked_amount()
-                .is_zero(),
+            get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone()).is_none(),
             "delegator 1 did not unstake full expected amount"
         );
         delegator_1_actual_payout
@@ -678,10 +680,7 @@ fn should_withdraw_bids_after_distribute() {
             undelegate_amount,
         );
         assert!(
-            get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_2.clone())
-                .expect("should still have zero bid")
-                .staked_amount()
-                .is_zero(),
+            get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_2.clone()).is_none(),
             "delegator 2 did not unstake full expected amount"
         );
         delegator_2_actual_payout
@@ -704,10 +703,7 @@ fn should_withdraw_bids_after_distribute() {
             withdraw_bid_amount,
         );
 
-        assert!(get_validator_bid(&mut builder, VALIDATOR_1.clone())
-            .expect("should still have zero bid")
-            .staked_amount()
-            .is_zero());
+        assert!(get_validator_bid(&mut builder, VALIDATOR_1.clone()).is_none());
 
         withdraw_bid_amount
     };
@@ -3169,10 +3165,7 @@ fn should_not_restake_after_full_unbond() {
         U512::from(DELEGATOR_1_STAKE),
     );
     let delegator = get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone());
-    assert!(delegator
-        .expect("should still have zero bid")
-        .staked_amount()
-        .is_zero());
+    assert!(delegator.is_none());
 
     let withdraws = builder.get_unbonds();
     let unbonding_purses = withdraws
@@ -3196,13 +3189,10 @@ fn should_not_restake_after_full_unbond() {
 
     builder.advance_era();
 
-    // Delegator's stake should remain at zero even though they were eligible for rewards in the
+    // Delegator should not remain delegated even though they were eligible for rewards in the
     // second era.
     let delegator = get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone());
-    assert!(delegator
-        .expect("should have zero bid")
-        .staked_amount()
-        .is_zero());
+    assert!(delegator.is_none());
 }
 
 // In this test, we set up a delegator and a validator, the delegator delegates to the validator.
@@ -3325,10 +3315,7 @@ fn delegator_full_unbond_during_first_reward_era() {
         U512::from(DELEGATOR_1_STAKE),
     );
     let delegator = get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone());
-    assert!(delegator
-        .expect("should still have zero bid")
-        .staked_amount()
-        .is_zero());
+    assert!(delegator.is_none());
 
     let withdraws = builder.get_unbonds();
     let unbonding_purses = withdraws
@@ -3351,8 +3338,5 @@ fn delegator_full_unbond_during_first_reward_era() {
     // Delegator's stake should remain at zero delegated even though they were eligible for rewards
     // in the second era.
     let delegator = get_delegator_bid(&mut builder, VALIDATOR_1.clone(), DELEGATOR_1.clone());
-    assert!(delegator
-        .expect("should still have zero bid")
-        .staked_amount()
-        .is_zero());
+    assert!(delegator.is_none());
 }

node/src/components/contract_runtime/operations.rs

@@ -715,6 +715,7 @@ pub fn execute_finalized_block(
         let step_processing_start = Instant::now();
 
         // force undelegate delegators outside delegation limits before the auction runs
+        debug!("starting forced undelegations");
         let forced_undelegate_req = ForcedUndelegateRequest::new(
             native_runtime_config.clone(),
             state_root_hash,
@@ -734,7 +735,9 @@ pub fn execute_finalized_block(
                 state_root_hash = post_state_hash;
             }
         }
+        debug!("forced undelegations success");
 
+        debug!("committing step");
         let step_effects = match commit_step(
             native_runtime_config,
             &scratch_state,
@@ -758,6 +761,7 @@ pub fn execute_finalized_block(
                 effects
             }
         };
+        debug!("step committed");
 
         let era_validators_req = EraValidatorsRequest::new(state_root_hash, protocol_version);
         let era_validators_result = data_access_layer.era_validators(era_validators_req);

node/src/reactor/main_reactor/keep_up.rs

@@ -594,6 +594,7 @@ impl MainReactor {
         match self.storage.get_highest_orphaned_block_header() {
             HighestOrphanedBlockResult::Orphan(highest_orphaned_block_header) => {
                 if let Some(synched) = self.synched(&highest_orphaned_block_header)? {
+                    debug!(?synched, "synched result");
                     return Ok(Some(synched));
                 }
                 let (sync_hash, sync_era) =
@@ -647,11 +648,16 @@ impl MainReactor {
             .map_err(|err| err.to_string())?
             .last()
         {
+            debug!(
+                highest_switch_timestamp=?highest_switch_block_header.timestamp(),
+                highest_orphaned_timestamp=?highest_orphaned_block_header.timestamp(),
+                "checking max ttl");
             let max_ttl: MaxTtl = self.chainspec.transaction_config.max_ttl.into();
             if max_ttl.synced_to_ttl(
                 highest_switch_block_header.timestamp(),
                 highest_orphaned_block_header,
             ) {
+                debug!("is synced to ttl");
                 return Ok(Some(SyncBackInstruction::TtlSynced));
             }
         }

node/src/reactor/main_reactor/tests.rs

@@ -1826,26 +1826,24 @@ async fn run_withdraw_bid_network() {
         .run_until_executed_transaction(&txn_hash, TEN_SECS)
         .await;
 
-    // Ensure execution succeeded and that there is a Write transform for the bid's key.
+    // Ensure execution succeeded and that there is a Prune transform for the bid's key.
     let bid_key = Key::BidAddr(BidAddr::from(alice_public_key.clone()));
     fixture
         .successful_execution_transforms(&txn_hash)
         .iter()
         .find(|transform| match transform.kind() {
-            TransformKindV2::Write(StoredValue::BidKind(bid_kind)) => {
-                Key::from(bid_kind.bid_addr()) == bid_key
-            }
+            TransformKindV2::Prune(prune_key) => prune_key == &bid_key,
             _ => false,
         })
-        .expect("should have a write record for bid");
+        .expect("should have a prune record for bid");
 
     // Crank the network forward until the era ends.
     fixture
         .run_until_stored_switch_block_header(ERA_ONE, ONE_MIN)
         .await;
 
-    // The bid record should still exist for now.
-    fixture.check_bid_existence_at_tip(&alice_public_key, None, true);
+    // The bid record should have been pruned once unbonding ran.
+    fixture.check_bid_existence_at_tip(&alice_public_key, None, false);
 
     // Crank the network forward until the unbonds are processed.
     fixture
@@ -1854,9 +1852,6 @@ async fn run_withdraw_bid_network() {
             ONE_MIN * 2,
         )
         .await;
-
-    // The bid record should have been pruned once unbonding ran.
-    fixture.check_bid_existence_at_tip(&alice_public_key, None, false);
 }
 
 #[tokio::test]
@@ -2035,27 +2030,25 @@ async fn run_undelegate_bid_network() {
         .run_until_executed_transaction(&txn_hash, TEN_SECS)
         .await;
 
-    // Ensure execution succeeded and that there is a Write transform for the bid's key.
+    // Ensure execution succeeded and that there is a Prune transform for the bid's key.
     fixture
         .successful_execution_transforms(&txn_hash)
         .iter()
         .find(|transform| match transform.kind() {
-            TransformKindV2::Write(StoredValue::BidKind(bid_kind)) => {
-                Key::from(bid_kind.bid_addr()) == bid_key
-            }
+            TransformKindV2::Prune(prune_key) => prune_key == &bid_key,
             _ => false,
         })
-        .expect("should have a write record for undelegated bid");
+        .expect("should have a prune record for undelegated bid");
 
     // Crank the network forward until the era ends.
     fixture
         .run_until_stored_switch_block_header(ERA_ONE, ONE_MIN)
         .await;
 
-    // Ensure the validator records are still present.
+    // Ensure the validator records are still present but the undelegated bid is gone.
     fixture.check_bid_existence_at_tip(&alice_public_key, None, true);
     fixture.check_bid_existence_at_tip(&bob_public_key, None, true);
-    fixture.check_bid_existence_at_tip(&bob_public_key, Some(&alice_public_key), true);
+    fixture.check_bid_existence_at_tip(&bob_public_key, Some(&alice_public_key), false);
 
     // Crank the network forward until the unbonds are processed.
     fixture
@@ -2064,11 +2057,6 @@ async fn run_undelegate_bid_network() {
             ONE_MIN * 2,
         )
         .await;
-
-    // Ensure the validator records are still present but the undelegated bid is gone.
-    fixture.check_bid_existence_at_tip(&alice_public_key, None, true);
-    fixture.check_bid_existence_at_tip(&bob_public_key, None, true);
-    fixture.check_bid_existence_at_tip(&bob_public_key, Some(&alice_public_key), false);
 }
 
 #[tokio::test]

storage/src/system/auction.rs

@@ -201,17 +201,15 @@ pub trait Auction:
                 let delegator_bid_addr =
                     BidAddr::new_from_public_keys(&public_key, Some(&delegator_public_key));
 
-                // Keep the bids for now - they will be pruned when the validator's unbonds get
-                // processed.
-                self.write_bid(
-                    delegator_bid_addr.into(),
-                    BidKind::Delegator(Box::new(delegator)),
-                )?;
+                debug!("pruning delegator bid {}", delegator_bid_addr);
+                self.prune_bid(delegator_bid_addr)
             }
+            debug!("pruning validator bid {}", validator_bid_addr);
+            self.prune_bid(validator_bid_addr);
+        } else {
+            self.write_bid(validator_bid_key, BidKind::Validator(validator_bid))?;
         }
 
-        self.write_bid(validator_bid_key, BidKind::Validator(validator_bid))?;
-
         Ok(updated_stake)
     }
 
@@ -299,8 +297,12 @@ pub trait Auction:
             updated_stake
         );
 
-        // Keep the bid for now - it will get pruned when the unbonds are processed.
-        self.write_bid(delegator_bid_addr.into(), BidKind::Delegator(delegator_bid))?;
+        if updated_stake.is_zero() {
+            debug!("pruning delegator bid {}", delegator_bid_addr);
+            self.prune_bid(delegator_bid_addr);
+        } else {
+            self.write_bid(delegator_bid_addr.into(), BidKind::Delegator(delegator_bid))?;
+        }
 
         Ok(updated_stake)
     }
@@ -548,6 +550,8 @@ pub trait Auction:
         include_credits: bool,
         credit_cap: Ratio<U512>,
     ) -> Result<(), ApiError> {
+        debug!("run_auction called");
+
         if self.get_caller() != PublicKey::System.to_account_hash() {
             return Err(Error::InvalidCaller.into());
         }
@@ -561,7 +565,9 @@ pub trait Auction:
         let mut era_id: EraId = detail::get_era_id(self)?;
 
         // Process unbond requests
+        debug!("processing unbond requests");
         detail::process_unbond_requests(self, max_delegators_per_validator)?;
+        debug!("processing unbond request successful");
 
         let mut validator_bids_detail = detail::get_validator_bids(self, era_id)?;
 
@@ -616,7 +622,6 @@ pub trait Auction:
                     .chain(
                         unlocked_validators
                             .into_iter()
-                            .filter(|(_, stake)| !stake.is_zero())
                             .take(remaining_auction_slots),
                     )
                     .collect()
@@ -656,6 +661,8 @@ pub trait Auction:
             detail::set_validator_bids(self, validator_bids)?;
         }
 
+        debug!("run_auction successful");
+
         Ok(())
     }