Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| @Test | ||
| void updateTransactionWithSegwitCompatible_before_rskip143() throws Exception { | ||
| SimpleBtcTransaction tx = (SimpleBtcTransaction) createSegwitTransaction(); | ||
| Set<Transaction> txs = new HashSet<>(); | ||
| txs.add(tx); | ||
|
|
||
| StoredBlock[] blocks = createBlockchain(4); | ||
| Block blockWithTx = createBlock(blocks[3].getHeader().getHash(), tx); | ||
|
|
||
| Map<Sha256Hash, Integer> appears = new HashMap<>(); | ||
| appears.put(blockWithTx.getHash(), 1); | ||
| tx.setAppearsInHashes(appears); | ||
|
|
||
| SimpleBitcoinWrapper bitcoinWrapper = new SimpleBitcoinWrapper(); | ||
| bitcoinWrapper.setTransactions(txs); | ||
| bitcoinWrapper.setBlocks(blocks); | ||
|
|
||
| SimpleFederatorSupport federatorSupport = new SimpleFederatorSupport(); | ||
|
|
||
| ActivationConfig activationsConfig = mock(ActivationConfig.class); | ||
| ActivationConfig.ForBlock activations = mock(ActivationConfig.ForBlock.class); | ||
| doReturn(activations).when(activationsConfig).forBlock(anyLong()); | ||
| doReturn(true).when(activations).isActive(ConsensusRule.RSKIP89); | ||
| doReturn(false).when(activations).isActive(ConsensusRule.RSKIP143); | ||
|
|
||
| BtcLockSenderProvider btcLockSenderProvider = mockBtcLockSenderProvider(TxSenderAddressType.P2SHP2WPKH); | ||
| int amountOfHeadersToSend = 100; | ||
|
|
||
| PowpegNodeSystemProperties config = mock(PowpegNodeSystemProperties.class); | ||
| when(config.getAmountOfHeadersToSend()).thenReturn(amountOfHeadersToSend); | ||
|
|
||
| BtcToRskClient client = btcToRskClientBuilder | ||
| .withActivationConfig(activationsConfig) | ||
| .withBitcoinWrapper(bitcoinWrapper) | ||
| .withFederatorSupport(federatorSupport) | ||
| .withBridgeConstants(bridgeRegTestConstants) | ||
| .withBtcLockSenderProvider(btcLockSenderProvider) | ||
| .withFederation(activeFederation) | ||
| .withFedNodeSystemProperties(config) | ||
| .build(); | ||
|
|
||
| client.onTransaction(tx); | ||
| client.onBlock(blockWithTx); | ||
|
|
||
| client.updateBridgeBtcTransactions(); | ||
|
|
||
| List<SimpleFederatorSupport.TransactionSentToRegisterBtcTransaction> txsSentToRegisterBtcTransaction = | ||
| federatorSupport.getTxsSentToRegisterBtcTransaction(); | ||
|
|
||
| assertNotNull(txsSentToRegisterBtcTransaction); | ||
| assertTrue(txsSentToRegisterBtcTransaction.isEmpty()); | ||
| } | ||
|
|
||
| @Test | ||
| void updateTransactionWithSenderUnknown_before_rskip170() throws Exception { | ||
| SimpleBtcTransaction tx = (SimpleBtcTransaction) createTransaction(); | ||
| Set<Transaction> txs = new HashSet<>(); | ||
| txs.add(tx); | ||
|
|
||
| StoredBlock[] blocks = createBlockchain(4); | ||
| Block blockWithTx = createBlock(blocks[3].getHeader().getHash(), tx); | ||
|
|
||
| Map<Sha256Hash, Integer> appears = new HashMap<>(); | ||
| appears.put(blockWithTx.getHash(), 1); | ||
| tx.setAppearsInHashes(appears); | ||
|
|
||
| SimpleBitcoinWrapper bitcoinWrapper = new SimpleBitcoinWrapper(); | ||
| bitcoinWrapper.setTransactions(txs); | ||
| bitcoinWrapper.setBlocks(blocks); | ||
|
|
||
| SimpleFederatorSupport federatorSupport = new SimpleFederatorSupport(); | ||
|
|
||
| ActivationConfig activationsConfig = mock(ActivationConfig.class); | ||
| ActivationConfig.ForBlock activations = mock(ActivationConfig.ForBlock.class); | ||
| doReturn(activations).when(activationsConfig).forBlock(anyLong()); | ||
| doReturn(true).when(activations).isActive(ConsensusRule.RSKIP89); | ||
| doReturn(true).when(activations).isActive(ConsensusRule.RSKIP143); | ||
| doReturn(false).when(activationsConfig).isActive(eq(ConsensusRule.RSKIP170), anyLong()); | ||
|
|
||
| BtcLockSenderProvider btcLockSenderProvider = mockBtcLockSenderProvider(TxSenderAddressType.UNKNOWN); | ||
| int amountOfHeadersToSend = 100; | ||
|
|
||
| PowpegNodeSystemProperties config = mock(PowpegNodeSystemProperties.class); | ||
| when(config.getAmountOfHeadersToSend()).thenReturn(amountOfHeadersToSend); | ||
|
|
||
| BtcToRskClient client = btcToRskClientBuilder | ||
| .withActivationConfig(activationsConfig) | ||
| .withBitcoinWrapper(bitcoinWrapper) | ||
| .withFederatorSupport(federatorSupport) | ||
| .withBridgeConstants(bridgeRegTestConstants) | ||
| .withBtcLockSenderProvider(btcLockSenderProvider) | ||
| .withFederation(activeFederation) | ||
| .withFedNodeSystemProperties(config) | ||
| .build(); | ||
|
|
||
| client.onTransaction(tx); | ||
| client.onBlock(blockWithTx); | ||
|
|
||
| client.updateBridgeBtcTransactions(); | ||
|
|
||
| List<SimpleFederatorSupport.TransactionSentToRegisterBtcTransaction> txsSentToRegisterBtcTransaction = | ||
| federatorSupport.getTxsSentToRegisterBtcTransaction(); | ||
|
|
||
| assertNotNull(txsSentToRegisterBtcTransaction); | ||
| assertTrue(txsSentToRegisterBtcTransaction.isEmpty()); | ||
| } | ||
|
|
There was a problem hiding this comment.
these _before_X_rskip tests dont make sense anymore
| assertNotNull(txsSentToRegisterBtcTransaction); | ||
| assertTrue(txsSentToRegisterBtcTransaction.isEmpty()); | ||
| } | ||
|
|
There was a problem hiding this comment.
julia-zack
force-pushed
the
refactor-btc-release-client-tests
branch
from
edd6c91 to
9864ae3
Compare
julia-zack
force-pushed
the
refactor-btc-release-client-tests
branch
from
9864ae3 to
b922e55
Compare
…factor to improve readability
| } | ||
|
|
||
| @Test | ||
| void updateTransactionWithSenderUnknown_after_rskip170() throws Exception { |
There was a problem hiding this comment.
this test was failing after the changes, as desired: the tx being created here had legacy pegin structure, and the lock sender was mocked to be UNKNOWN.
And now we don't wanna send these txs.
Removing instead of modifying since we already have this case covered by new tests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| long bestBlockNumber = federatorSupport.getRskBestChainHeight(); | ||
| ActivationConfig.ForBlock activations = activationConfig.forBlock(bestBlockNumber); | ||
|
|
||
| if (PegUtilsLegacy.isPegOutTx(btcTx, List.of(federation), activations)) { |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, fixing this means replacing the deprecated PegUtilsLegacy.isPegOutTx(...) call with the modern, non‑deprecated peg‑out detection API described in the @deprecated Javadoc of PegUtilsLegacy.isPegOutTx. The new API is almost certainly in a non‑legacy peg utility class (commonly something like BridgeUtils or PegUtils) and will provide a method that performs the same check, possibly with a slightly different signature (for example, by taking a Federation instead of a List<Federation>, or omitting the activations argument if it can obtain it internally).
The best way to fix this without changing functionality is:
- Identify the direct, non‑deprecated replacement for
PegUtilsLegacy.isPegOutTx(BtcTransaction, List<Federation>, ActivationConfig.ForBlock). In this codebase, there is already an import ofco.rsk.peg.BridgeUtils, so the replacement is very likelyBridgeUtils.isPegOutTx(btcTx, List.of(federation), activations)or a very close variant. Because we cannot see the rest of the code, we must assume the simplest compatible change: swappingPegUtilsLegacywithBridgeUtilswhile preserving arguments. - Update line 717 to call the non‑deprecated method:
BridgeUtils.isPegOutTx(btcTx, List.of(federation), activations). - No import changes are needed because
BridgeUtilsis already imported on line 14. - No other behavior in
shouldSendTxchanges: the logical conditions and branching remain identical; only the internal implementation used to decide “peg‑out tx” is switched to the modern, supported helper.
So the concrete change is in src/main/java/co/rsk/federate/BtcToRskClient.java at the if (PegUtilsLegacy.isPegOutTx(...)) line, replacing PegUtilsLegacy with BridgeUtils.
| @@ -714,7 +714,7 @@ | ||
| long bestBlockNumber = federatorSupport.getRskBestChainHeight(); | ||
| ActivationConfig.ForBlock activations = activationConfig.forBlock(bestBlockNumber); | ||
|
|
||
| if (PegUtilsLegacy.isPegOutTx(btcTx, List.of(federation), activations)) { | ||
| if (BridgeUtils.isPegOutTx(btcTx, List.of(federation), activations)) { | ||
| return true; | ||
| } | ||
|
|
| private boolean shouldSendTx(Transaction tx, co.rsk.bitcoinj.wallet.Wallet federationWallet) { | ||
| BtcTransaction btcTx = ThinConverter.toThinInstance(bridgeConstants.getBtcParams(), tx); | ||
|
|
||
| co.rsk.bitcoinj.core.Coin valueSentToMe = btcTx.getValueSentToMe(federationWallet); | ||
| if (valueSentToMe.isZero()) { | ||
| return false; | ||
| } | ||
| long bestBlockNumber = federatorSupport.getRskBestChainHeight(); | ||
| ActivationConfig.ForBlock activations = activationConfig.forBlock(bestBlockNumber); | ||
|
|
||
| if (PegUtilsLegacy.isPegOutTx(btcTx, List.of(federation), activations)) { | ||
| return true; | ||
| } | ||
|
|
||
| // from now on, the tx will be treated as a pegin | ||
| boolean isAnyValueSentBelowMinimumPeginValue = !allUTXOsToFedAreAboveMinimumPeginValue( | ||
| btcTx, | ||
| federationWallet, | ||
| bridgeConstants.getMinimumPeginTxValue(activations) | ||
| ); | ||
| if (isAnyValueSentBelowMinimumPeginValue) { | ||
| return false; | ||
| } | ||
|
|
||
| PeginInformation peginInformation = new PeginInformation( | ||
| btcLockSenderProvider, | ||
| peginInstructionsProvider, | ||
| activations | ||
| ); | ||
| try { | ||
| peginInformation.parse(btcTx); | ||
| } catch (PeginInstructionsException e) { | ||
| // pegin instructions exception is only being thrown when a pegin has an INVALID op return output structure | ||
| // meaning if there's NO op return found, or the protocol version is unknown, it won't throw. | ||
| co.rsk.bitcoinj.core.Address refundAddress = peginInformation.getSenderBtcAddress(); | ||
| // if there's a refund address, i.e., refundAddress is not null, tx should be sent | ||
| return refundAddress != null; | ||
| } | ||
|
|
||
| if (isLegacyPegin(peginInformation)) { | ||
| // if there's a valid lock sender, i.e., sender address type is not UNKNOWN, tx should be sent | ||
| return peginInformation.getSenderBtcAddressType() != BtcLockSender.TxSenderAddressType.UNKNOWN; | ||
| } | ||
|
|
||
| // at this point, the tx is either a legacy pegin nor a pegin V1 that should be refund. | ||
| // so we just want to send it if it's a pegin V1 (that would be valid at this point) | ||
| return isPeginV1(peginInformation); | ||
| } |
There was a problem hiding this comment.
notice: using the exact same checks as BitcoinWrapperImpl before adding txs to file is not enough, since it considers (for example) a pegin from an unknown protocol as a valid one.
Eventually, this should be moved to a new utility class to be reused from BitcoinWrapperImpl and BtcToRskClient. And in that new utility class we should stop using PegUtilsLegacy.
but for now, this should be enough to not break too many things but also improve readability and checks.
| "[updateBridgeBtcTransactions] Btc tx {} was not found in wallet or is not yet confirmed.", | ||
| txHash | ||
| ); | ||
| // Don't remove it as we still have to wait for its confirmations. |
There was a problem hiding this comment.
Why remove this comment? I feel like it was useful
There was a problem hiding this comment.
oh i think i removed it by mistake. I'll put it back again
| return peginInformation.getSenderBtcAddressType() != BtcLockSender.TxSenderAddressType.UNKNOWN; | ||
| } | ||
|
|
||
| // at this point, the tx is either a legacy pegin nor a pegin V1 that should be refund. |
There was a problem hiding this comment.
This sounds a bit confusing
There was a problem hiding this comment.
I rephrased comments and rename method, check now: 5710139
| // Tx could be null if having less than the desired amount of confirmations, | ||
| // do not clear in that case since we'd leave a tx without processing | ||
| } else { | ||
| if (federatorSupport.isBtcTxHashAlreadyProcessed(tx.getTxId())) { |
There was a problem hiding this comment.
Not sure if it makes a big difference, but perhaps we should have this check before shouldSendTx? If tx is already informed and will be removed no need to perform all the checks that shouldSendTx does
There was a problem hiding this comment.
since that is just a contract call, yes, you are right.
Fixed: 75debbe
| continue; | ||
| } | ||
|
|
||
| sendTx(tx, txHash); |
There was a problem hiding this comment.
Can't the txHash be fetched from the tx? Or is it a different hash?
There was a problem hiding this comment.
you right, i created a private method to get the correct hash (wtxid wen segwit, txid wen legacy). Thanks!
| if (proofs == null || proofs.isEmpty()) { | ||
| logger.debug("[updateBridgeBtcTransactions] No proofs found for tx {}", txHash); | ||
| return; | ||
| } |
There was a problem hiding this comment.
And what happens here? Will the transaction ever be able to be informed?
There was a problem hiding this comment.
honestly i'm not sure why these checks are being performed at this point, pretty sure if we got here we already know the proof exists. But it prob need deeper analysis, so i'd just reordered this block of code.
| StoredBlock txStoredBlock = findBestChainStoredBlockFor(tx); | ||
| int blockHeight = txStoredBlock.getHeight(); | ||
| logger.debug( | ||
| "[updateBridgeBtcTransactions] Tx {} belongs to block {} at height {}", | ||
| txHash, | ||
| txStoredBlock.getHeader().getHash(), | ||
| blockHeight | ||
| ); |
There was a problem hiding this comment.
Just an idea.
I'd move this to the top, then have all the logic to get the proofs and build the pmt together (maybe extract to a private method), finally send the transaction.
There was a problem hiding this comment.
seems like a good idea, thanks
| private static final BtcECKey SENDER_PUBLIC_KEY = BitcoinTestUtils.getBtcEcKeyFromSeed("sender"); | ||
| private static final List<BtcECKey> MULTISIG_KEYS = Arrays.asList( | ||
| BitcoinTestUtils.getBtcEcKeyFromSeed("key1"), | ||
| BitcoinTestUtils.getBtcEcKeyFromSeed("key2"), | ||
| BitcoinTestUtils.getBtcEcKeyFromSeed("key3") | ||
| ); |
There was a problem hiding this comment.
Why reference the class and not call those methods directly?
julia-zack
force-pushed
the
refactor-btc-release-client-tests
branch
from
b1d5637 to
46f2866
Compare
julia-zack
force-pushed
the
refactor-btc-release-client-tests
branch
from
46f2866 to
fc5f05f
Compare
|
2 participants
No description provided.