Switch to compounding when consolidating with source==target #14511

terencechain · 2024-10-07T14:24:59Z

This PR modifies the handling of switching validators to compounding credentials by consolidating the logic into the process_consolidation_request flow. Key code changes include:

Removed switch_to_compounding_validator from process_pending_consolidations.
Removed switch_to_compounding_validator from apply_deposit.
Added a helper function is_valid_switch_to_compounding_request.
Updated process_consolidation_request to check is_valid_switch_to_compounding_request, and if valid, switch the source index validator to compounding.
Updated process_consolidation_request to automatically switch the target validator to compounding if it has an eth1 credential.

Note to the reviewers: In process_consolidation_request, we used to return errors in many places, but in the spec, it's a clean return. Since we process requests in one function, whereas the spec processes individual requests, returning errors is dangerous and could lead to a consensus fault. I've adjusted some returns to log and continue. I didn't modify the ones where it says an error can't happen. Please review carefully

Reference: ethereum/consensus-specs#3918

james-prysm · 2024-10-14T21:16:22Z

beacon-chain/core/electra/consolidations.go

 		if err != nil {
 			return err
 		}
-		if err := helpers.DecreaseBalance(st, pc.SourceIndex, activeBalance); err != nil {
+		if sourceEffectiveBalance > validatorBalance {


nit: instead of this if statement what about using the min function for 1 to 1 readability

james-prysm · 2024-10-14T21:22:37Z

beacon-chain/core/electra/consolidations.go

+
+	sourcePubKey := bytesutil.ToBytes48(req.SourcePubkey)
+	targetPubKey := bytesutil.ToBytes48(req.TargetPubkey)
+	if sourcePubKey != targetPubKey {


I think it's better to do !bytes.Equals check than doing !=

james-prysm · 2024-10-14T21:27:14Z

beacon-chain/core/electra/consolidations.go

+		}
+		sourcePubkey := bytesutil.ToBytes48(cr.SourcePubkey)
+		targetPubkey := bytesutil.ToBytes48(cr.TargetPubkey)
+		if sourcePubkey == targetPubkey {


another check for bytes.Equal over ==

prestonvanloon · 2024-10-14T21:35:53Z

beacon-chain/core/electra/consolidations.go

+		sourcePubkey := bytesutil.ToBytes48(cr.SourcePubkey)
+		targetPubkey := bytesutil.ToBytes48(cr.TargetPubkey)
+		if sourcePubkey == targetPubkey {


+1, bytes.Equal does not alloc, but converting to [48]byte is a 48 byte alloc

We would still need [48]byte when calling ValidatorIndexByPubkey in L224

srcIdx, ok := st.ValidatorIndexByPubkey(sourcePubkey) tgtIdx, ok := st.ValidatorIndexByPubkey(targetPubkey)

prestonvanloon · 2024-10-14T21:38:37Z

beacon-chain/core/electra/consolidations.go

-//	        active_balance = get_active_balance(state, pending_consolidation.source_index)
-//	        decrease_balance(state, pending_consolidation.source_index, active_balance)
-//	        increase_balance(state, pending_consolidation.target_index, active_balance)
+//	continue


prestonvanloon · 2024-10-14T22:06:54Z

beacon-chain/core/electra/consolidations.go

-//	        increase_balance(state, pending_consolidation.target_index, active_balance)
+//	continue
+//
+// def process_pending_consolidations(state: BeaconState) -> None:


Double checked this against spec, looks good.

prestonvanloon · 2024-10-14T22:10:12Z

beacon-chain/core/electra/consolidations.go

@@ -101,6 +107,16 @@ func ProcessPendingConsolidations(ctx context.Context, st state.BeaconState) err
 //	    state: BeaconState,
 //	    consolidation_request: ConsolidationRequest
 //	) -> None:
+//	    if is_valid_switch_to_compounding_request(state, consolidation_request):


Double checked this comment block against spec and LGTM.

prestonvanloon · 2024-10-14T22:17:28Z

beacon-chain/core/electra/consolidations.go

+//	if not is_active_validator(source_validator, current_epoch):
+//	    return False
+//
+//	# Verify exit for source have not been initiated


Suggested change

// # Verify exit for source have not been initiated

// # Verify exit for source has not been initiated

The spec has this typo corrected

prestonvanloon · 2024-10-14T22:19:14Z

beacon-chain/core/electra/validator.go

-//	if has_eth1_withdrawal_credential(validator):
-//		validator.withdrawal_credentials = COMPOUNDING_WITHDRAWAL_PREFIX + validator.withdrawal_credentials[1:]
-//		queue_excess_active_balance(state, index)
+//	validator.withdrawal_credentials = COMPOUNDING_WITHDRAWAL_PREFIX + validator.withdrawal_credentials[1:]


Review this against spec and LGTM.

prestonvanloon · 2024-10-15T01:37:07Z

beacon-chain/core/electra/consolidations.go

+	}
+	// As per the consensus specification, this error is not considered an assertion.
+	// Therefore, if the source_pubkey is not found in validator_pubkeys, we simply return false.
+	srcV, err := st.ValidatorAtIndex(srcIdx)


Can you use ValidatorAtIndexReadOnly?

terencechain force-pushed the consolidation-switch-to-comp1 branch 3 times, most recently from a4b49a9 to 177febd Compare October 14, 2024 20:31

terencechain marked this pull request as ready for review October 14, 2024 20:32

terencechain requested a review from a team as a code owner October 14, 2024 20:32

terencechain requested review from nalepae, saolyn and potuz October 14, 2024 20:32

terencechain force-pushed the consolidation-switch-to-comp1 branch from 177febd to bf95096 Compare October 14, 2024 20:35

terencechain added the Ready For Review A pull request ready for code review label Oct 14, 2024

james-prysm reviewed Oct 14, 2024

View reviewed changes

prestonvanloon reviewed Oct 14, 2024

View reviewed changes

Switch to compounding when consolidating with source==target

68b6526

terencechain force-pushed the consolidation-switch-to-comp1 branch from bf95096 to 68b6526 Compare October 15, 2024 01:34

prestonvanloon reviewed Oct 15, 2024

View reviewed changes

Feedback

2d64dc6

prestonvanloon approved these changes Oct 15, 2024

View reviewed changes

james-prysm approved these changes Oct 15, 2024

View reviewed changes

terencechain added this pull request to the merge queue Oct 15, 2024

Merged via the queue into develop with commit 0a4ed82 Oct 15, 2024
17 of 18 checks passed

terencechain deleted the consolidation-switch-to-comp1 branch October 15, 2024 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to compounding when consolidating with source==target #14511

Switch to compounding when consolidating with source==target #14511

terencechain commented Oct 7, 2024 •

edited

Loading

james-prysm Oct 14, 2024

terencechain Oct 15, 2024

james-prysm Oct 14, 2024

james-prysm Oct 14, 2024

terencechain Oct 15, 2024

prestonvanloon Oct 14, 2024

terencechain Oct 15, 2024

prestonvanloon Oct 14, 2024

terencechain Oct 15, 2024

prestonvanloon Oct 14, 2024

prestonvanloon Oct 14, 2024

prestonvanloon Oct 14, 2024

prestonvanloon Oct 14, 2024

prestonvanloon Oct 15, 2024

terencechain Oct 15, 2024

	// # Verify exit for source have not been initiated
	// # Verify exit for source has not been initiated

Switch to compounding when consolidating with source==target #14511

Switch to compounding when consolidating with source==target #14511

Conversation

terencechain commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain commented Oct 7, 2024 •

edited

Loading