Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark invalid pegin as processed #2910

Merged
merged 5 commits into from
Jan 10, 2025
Merged

Mark invalid pegin as processed #2910

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aca9920
Mark rejected pegins as processed once RSKIP459 is active
nathanieliov Dec 20, 2024
208b14f
- Rename CANNOT_BE_PROCESSED entry to CANNOT_BE_REFUNDED since now re…
nathanieliov Dec 31, 2024
b2a1d48
- Use new switch with arrow labels
nathanieliov Jan 7, 2025
61bd76f
Improve switch use to handle pegins
nathanieliov Jan 7, 2025
bf15222
- Rename UnrefundablePeginReason to NonRefundablePeginReason
nathanieliov Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 35 additions & 17 deletions rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,33 +503,50 @@ protected void registerPegIn(
);

PeginProcessAction peginProcessAction = peginEvaluationResult.getPeginProcessAction();

if (peginProcessAction == PeginProcessAction.CAN_BE_REGISTERED) {
logger.debug("[{}] Peg-in is valid, going to register", METHOD_NAME);
executePegIn(btcTx, peginInformation, totalAmount);
} else {
Optional<RejectedPeginReason> rejectedPeginReasonOptional = peginEvaluationResult.getRejectedPeginReason();
if (rejectedPeginReasonOptional.isEmpty()) {
return;
}

// If the peg-in cannot be registered means it should be rejected
RejectedPeginReason rejectedPeginReason = peginEvaluationResult.getRejectedPeginReason()
.map(reason -> {
logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, reason);
eventLogger.logRejectedPegin(btcTx, reason);
return reason;
}).orElseThrow(() -> {
// This flow should never be reached. There should always be a rejected pegin reason.
String message = "Invalid state. No rejected reason was returned from evaluatePegin method";
logger.error("[{}}] {}", METHOD_NAME, message);
throw new IllegalStateException(message);
String message = "Invalid state. No rejected reason was returned for an invalid pegin.";
logger.error("[{}] {}", METHOD_NAME, message);
return new IllegalStateException(message);
});

switch (peginProcessAction) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use here the new switch expression with arrow labels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

case CAN_BE_REFUNDED -> {
logger.debug("[{}] Refunding to address {} ", METHOD_NAME,
peginInformation.getBtcRefundAddress());
generateRejectionRelease(btcTx, peginInformation.getBtcRefundAddress(), rskTxHash,
totalAmount);
markTxAsProcessed(btcTx);
}
case CANNOT_BE_REFUNDED -> {
logger.debug("[{}] Nonrefundable transaction {}.", METHOD_NAME, btcTx.getHash());
handleNonRefundablePegin(btcTx, peginInformation.getProtocolVersion(),
rejectedPeginReason);

if (!activations.isActive(RSKIP459)) {
return;
}

RejectedPeginReason rejectedPeginReason = rejectedPeginReasonOptional.get();
logger.debug("[{}] Rejected peg-in, reason {}", METHOD_NAME, rejectedPeginReason);
eventLogger.logRejectedPegin(btcTx, rejectedPeginReason);
if (peginProcessAction == PeginProcessAction.CAN_BE_REFUNDED) {
logger.debug("[{}] Refunding to address {} ", METHOD_NAME, peginInformation.getBtcRefundAddress());
generateRejectionRelease(btcTx, peginInformation.getBtcRefundAddress(), rskTxHash, totalAmount);
// Since RSKIP459, rejected peg-ins should be marked as processed
markTxAsProcessed(btcTx);
} else {
logger.debug("[{}] Unprocessable transaction {}.", METHOD_NAME, btcTx.getHash());
handleUnprocessableBtcTx(btcTx, peginInformation.getProtocolVersion(), rejectedPeginReason);
}
}
}

private void handleUnprocessableBtcTx(
private void handleNonRefundablePegin(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the same renaming idea could apply here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

BtcTransaction btcTx,
int protocolVersion,
RejectedPeginReason rejectedPeginReason
Expand All @@ -543,7 +560,8 @@ private void handleUnprocessableBtcTx(
UnrefundablePeginReason.LEGACY_PEGIN_UNDETERMINED_SENDER;
}

logger.debug("[handleUnprocessableBtcTx] Unprocessable tx {}. Reason {}", btcTx.getHash(), unrefundablePeginReason);
logger.debug("[handleNonRefundablePegin] Nonrefundable tx {}. Reason {}", btcTx.getHash(),
unrefundablePeginReason);
eventLogger.logUnrefundablePegin(btcTx, unrefundablePeginReason);
}

Expand Down
6 changes: 3 additions & 3 deletions rskj-core/src/main/java/co/rsk/peg/PegUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ static PeginEvaluationResult evaluatePegin(

if(!allUTXOsToFedAreAboveMinimumPeginValue(btcTx, fedWallet, minimumPeginTxValue, activations)) {
logger.debug("[evaluatePegin] Peg-in contains at least one utxo below the minimum value");
return new PeginEvaluationResult(PeginProcessAction.CANNOT_BE_PROCESSED, INVALID_AMOUNT);
return new PeginEvaluationResult(PeginProcessAction.CANNOT_BE_REFUNDED, INVALID_AMOUNT);
}

try {
Expand All @@ -197,7 +197,7 @@ static PeginEvaluationResult evaluatePegin(

PeginProcessAction peginProcessAction = hasRefundAddress ?
PeginProcessAction.CAN_BE_REFUNDED :
PeginProcessAction.CANNOT_BE_PROCESSED;
PeginProcessAction.CANNOT_BE_REFUNDED;

return new PeginEvaluationResult(peginProcessAction, PEGIN_V1_INVALID_PAYLOAD);
}
Expand Down Expand Up @@ -225,7 +225,7 @@ private static PeginEvaluationResult evaluateLegacyPeginSender(TxSenderAddressTy
case P2SHP2WSH:
return new PeginEvaluationResult(PeginProcessAction.CAN_BE_REFUNDED, LEGACY_PEGIN_MULTISIG_SENDER);
default:
return new PeginEvaluationResult(PeginProcessAction.CANNOT_BE_PROCESSED, LEGACY_PEGIN_UNDETERMINED_SENDER);
return new PeginEvaluationResult(PeginProcessAction.CANNOT_BE_REFUNDED, LEGACY_PEGIN_UNDETERMINED_SENDER);
}
}

Expand Down
2 changes: 1 addition & 1 deletion rskj-core/src/main/java/co/rsk/peg/pegin/PeginProcessAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@
public enum PeginProcessAction {
CAN_BE_REGISTERED,
CAN_BE_REFUNDED,
CANNOT_BE_PROCESSED
CANNOT_BE_REFUNDED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, to rename this for clarity

REGISTER,
REFUND,
NO_REFUND

What do you think? A bit more straightforward. Last one could also be IGNORE perhaps, not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks simpler. I'll update it.

}
43 changes: 27 additions & 16 deletions rskj-core/src/test/java/co/rsk/peg/BridgeSupportRegisterBtcTransactionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import java.util.stream.Stream;
import org.bouncycastle.util.encoders.Hex;
import org.ethereum.config.blockchain.upgrades.*;
import org.ethereum.config.blockchain.upgrades.ActivationConfig.ForBlock;
import org.ethereum.core.*;
import org.ethereum.crypto.ECKey;
import org.ethereum.vm.PrecompiledContracts;
Expand Down Expand Up @@ -108,11 +109,13 @@ private void assertInvalidPeginIsIgnored() throws IOException {
}

// After peg-out tx index gets in use
private void assertInvalidPeginIsRejectedWithInvalidAmountReason(BtcTransaction btcTransaction) throws IOException {
private void assertInvalidPeginIsRejectedWithInvalidAmountReason(BtcTransaction btcTransaction, ActivationConfig.ForBlock activations) throws IOException {
verify(bridgeEventLogger, times(1)).logRejectedPegin(btcTransaction, INVALID_AMOUNT);
verify(bridgeEventLogger, times(1)).logUnrefundablePegin(btcTransaction, UnrefundablePeginReason.INVALID_AMOUNT);
verify(bridgeEventLogger, never()).logPeginBtc(any(), any(), any(), anyInt());
verify(provider, never()).setHeightBtcTxhashAlreadyProcessed(any(), anyLong());

var shouldMarkTxAsProcessed = activations == lovell700Activations? times(1) : never();
verify(provider, shouldMarkTxAsProcessed).setHeightBtcTxhashAlreadyProcessed(any(), anyLong());
assertTrue(activeFederationUtxos.isEmpty());
assertTrue(retiringFederationUtxos.isEmpty());
}
Expand Down Expand Up @@ -194,7 +197,13 @@ private void assertLegacyUndeterminedSenderPeginIsRejectedAsPeginV1InvalidPayloa
}

// After arrowhead600Activations is activated
private void assertLegacyUndeterminedSenderPeginIsRejected(BtcTransaction btcTransaction) throws IOException {
private void assertLegacyUndeterminedSenderPeginIsRejected(BtcTransaction btcTransaction,
ForBlock activations) throws IOException {

// tx should be marked as processed since RSKIP459 is active
var shouldMarkTxAsProcessed = activations == lovell700Activations? times(1) : never();
verify(provider, shouldMarkTxAsProcessed).setHeightBtcTxhashAlreadyProcessed(any(), anyLong());

verify(bridgeEventLogger, times(1)).logRejectedPegin(
btcTransaction, RejectedPeginReason.LEGACY_PEGIN_UNDETERMINED_SENDER
);
Expand All @@ -205,14 +214,18 @@ private void assertLegacyUndeterminedSenderPeginIsRejected(BtcTransaction btcTra

verify(bridgeEventLogger, never()).logPeginBtc(any(), any(), any(), anyInt());
verify(bridgeEventLogger, never()).logReleaseBtcRequested(any(), any(), any());
verify(provider, never()).setHeightBtcTxhashAlreadyProcessed(any(), anyLong());

Assertions.assertTrue(activeFederationUtxos.isEmpty());
Assertions.assertTrue(retiringFederationUtxos.isEmpty());
Assertions.assertTrue(pegoutsWaitingForConfirmations.getEntries().isEmpty());
}

private void assertInvalidPeginV1UndeterminedSenderIsRejected(BtcTransaction btcTransaction) throws IOException {
private void assertInvalidPeginV1UndeterminedSenderIsRejected(BtcTransaction btcTransaction,
ForBlock activations) throws IOException {

var shouldMarkTxAsProcessed = activations == lovell700Activations? times(1) : never();
verify(provider, shouldMarkTxAsProcessed).setHeightBtcTxhashAlreadyProcessed(any(), anyLong());

verify(bridgeEventLogger, times(1)).logRejectedPegin(
btcTransaction, PEGIN_V1_INVALID_PAYLOAD
);
Expand All @@ -224,7 +237,6 @@ private void assertInvalidPeginV1UndeterminedSenderIsRejected(BtcTransaction btc
verify(bridgeEventLogger, never()).logPeginBtc(any(), any(), any(), anyInt());
verify(bridgeEventLogger, never()).logReleaseBtcRequested(any(), any(), any());
verify(bridgeEventLogger, never()).logPegoutTransactionCreated(any(), any());
verify(provider, never()).setHeightBtcTxhashAlreadyProcessed(any(), anyLong());

assertTrue(activeFederationUtxos.isEmpty());
assertTrue(retiringFederationUtxos.isEmpty());
Expand Down Expand Up @@ -932,7 +944,7 @@ void pegin_to_active_fed_below_minimum(

// assert
if (shouldUsePegoutTxIndex) {
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction);
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction, activations);
} else {
assertInvalidPeginIsIgnored();
}
Expand Down Expand Up @@ -970,7 +982,7 @@ void pegin_to_active_fed_below_and_above_minimum(

// assert
if (shouldUsePegoutTxIndex) {
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction);
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction, activations);
} else {
assertInvalidPeginIsIgnored();
}
Expand Down Expand Up @@ -1012,7 +1024,7 @@ void pegin_multiple_outputs_to_active_fed_sum_amount_equal_to_minimum_pegin(

// assert
if (shouldUsePegoutTxIndex) {
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction);
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction, activations);
} else {
assertInvalidPeginIsIgnored();
}
Expand Down Expand Up @@ -1085,7 +1097,7 @@ void pegin_to_active_fed_below_minimum_and_retiring_above_minimum(

// assert
if (shouldUsePegoutTxIndex) {
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction);
assertInvalidPeginIsRejectedWithInvalidAmountReason(btcTransaction, activations);
} else {
assertInvalidPeginIsIgnored();
}
Expand Down Expand Up @@ -1405,7 +1417,7 @@ void pegin_v1_to_active_fed_with_invalid_payload_and_unknown_sender_cannot_be_pr
if (activations == fingerrootActivations){
assertLegacyUndeterminedSenderPeginIsRejectedAsPeginV1InvalidPayloadBeforeRSKIP379(btcTransaction);
} else {
assertInvalidPeginV1UndeterminedSenderIsRejected(btcTransaction);
assertInvalidPeginV1UndeterminedSenderIsRejected(btcTransaction, activations);
}
}

Expand Down Expand Up @@ -1494,7 +1506,7 @@ void pegin_to_retiring_fed_cannot_be_processed(
if (activations == fingerrootActivations){
assertLegacyUndeterminedSenderPeginIsRejectedAsPeginV1InvalidPayloadBeforeRSKIP379(btcTransaction);
} else {
assertLegacyUndeterminedSenderPeginIsRejected(btcTransaction);
assertLegacyUndeterminedSenderPeginIsRejected(btcTransaction, activations);
}
}

Expand Down Expand Up @@ -1543,12 +1555,11 @@ void pegin_legacy_from_segwit_to_active_fed_cannot_be_processed(
);

// assert

// SINCE RSKIP379 ONLY TRANSACTIONS THAT REALLY ARE PROCESSED, REFUNDS OR REGISTER WILL BE MARK AS PROCESSED.
if (activations == fingerrootActivations){
if (activations == fingerrootActivations) {
// BEFORE RSKIP379 REJECTED PEGIN WERE MARKED AS PROCESSED.
assertLegacyUndeterminedSenderPeginIsRejectedAsPeginV1InvalidPayloadBeforeRSKIP379(btcTransaction);
} else {
assertLegacyUndeterminedSenderPeginIsRejected(btcTransaction);
assertLegacyUndeterminedSenderPeginIsRejected(btcTransaction, activations);
}
}

Expand Down
Loading
Loading