Showcase the Bridge behaviour under different scenarios #2633

apancorb · 2024-07-23T14:59:11Z

Description

Adds unit tests to analyze a new pegout minimum. This PR is just focused on analisis purposes.

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

Other information:

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

apancorb · 2024-07-23T15:04:10Z

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

+        Coin minSpendableUTXOValue = Coin.SATOSHI;
+        tx.clearOutputs();
+        tx.addOutput(minSpendableUTXOValue, fed.getP2SHScript());
+        while (tx.getOutput(0).isDust()) {
+            minSpendableUTXOValue = minSpendableUTXOValue.add(Coin.SATOSHI);
+            tx.clearOutputs();
+            tx.addOutput(minSpendableUTXOValue, fed.getP2SHScript());
+        }


Interesting enough this value turns out to be 2700 SATS. Which is exactly the lowest value UTXO I was able to find in the Bridge address. Meaning given the current wallet implementation, this is the lowest value that the Bridge will not consider it as dust and will proceed to spend it if so desired by the selector UTXO algorithm.

Here is the tx: https://mempool.space/tx/ab94ce7abfe178025b593ce0f0c7468a984087ae7977800b62cf47b4284d00dd

So this is the lowest change output the Bridge will create, right? If a UTXO is used that generates a smaller change, it will remove sats from the user output until it completes 2,700 sats as the change. So we could say that the max it will drain from a user output is 2,700 sats in the case that the change was 0.

And this does not depend on the feePerKb value set in the Bridge, is that right?

So this is the lowest change output the Bridge will create, right? If a UTXO is used that generates a smaller change, it will remove sats from the user output until it completes 2,700 sats as the change. So we could say that the max it will drain from a user output is 2,700 sats in the case that the change was 0.

That is right.

And this does not depend on the feePerKb value set in the Bridge, is that right?

That is right, reason being because it always uses a hardcoded feePerKb value of 15,000 SATS

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

apancorb · 2024-07-23T15:11:17Z

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

+        );
+
+        // build batch peg-out transaction
+        ReleaseTransactionBuilder.BuildResult result = releaseTransactionBuilder.buildBatchedPegouts(entries);


With the current feePerKb of 24,000 SATS and my initial proposed minPegoutValue of 28,500 SATS, this batch peg-out transaction will not be built with a Wallet.CouldNotAdjustDownwards exception. Meaning the user output could not be adjusted downwards to pay the tx fees. This makes sense since this scenario I believe is the worst case scenario, since we have N P2SH inputs with the min possible value to be non-dust and only one user output that is going to take all the load. So I think if we can find two values that pass this scenario, then we should be good for any other scenario. Wdyt @marcos-iov ?

Nevermind, this test scenario will never pass no matter what min peg-out value you choose. Maybe we can just split the min peg-out value by using 10 inputs. Like if min peg-out value is 20 then have 10 inputs of 2 each. Wdyt?

Not sure I followed the logic here. There is only one pegout request for the min pegout value, right? And how many UTXOs available and for what value?

Yes only one pegout request. And there will be as many UTXOs available to cover the request. Each UTXO will have a value of 2,700 SATS. The problem with this test is that is impossible to pass since it is too much fees that has to be taken from the user. I have written a similar one but that takes the pegout request value and divides it by 10 equal UTXOs.

marcos-iov

Should we also consider the size of the transaction after signing?

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

marcos-iov · 2024-07-31T20:23:28Z

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

+            mock(BridgeStorageProvider.class)
+        );
+        // build release transaction builder for current fed
+        when(activations.isActive(ConsensusRule.RSKIP201)).thenReturn(true);


Just in case, let's use ActivationConfigForTest class. And get the all object, that includes all RSKIPs active

How would it look in code? If I understand correctly, I should still mock isActive(ConsensusRule.RSKIP201) right?

marcos-iov · 2024-07-31T20:28:58Z

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

+        Coin minSpendableUTXOValue = Coin.SATOSHI;
+        tx.clearOutputs();
+        tx.addOutput(minSpendableUTXOValue, fed.getP2SHScript());
+        while (tx.getOutput(0).isDust()) {
+            minSpendableUTXOValue = minSpendableUTXOValue.add(Coin.SATOSHI);
+            tx.clearOutputs();
+            tx.addOutput(minSpendableUTXOValue, fed.getP2SHScript());
+        }


So this is the lowest change output the Bridge will create, right? If a UTXO is used that generates a smaller change, it will remove sats from the user output until it completes 2,700 sats as the change. So we could say that the max it will drain from a user output is 2,700 sats in the case that the change was 0.

And this does not depend on the feePerKb value set in the Bridge, is that right?

marcos-iov · 2024-07-31T20:32:11Z

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

+        );
+
+        // build batch peg-out transaction
+        ReleaseTransactionBuilder.BuildResult result = releaseTransactionBuilder.buildBatchedPegouts(entries);


Not sure I followed the logic here. There is only one pegout request for the min pegout value, right? And how many UTXOs available and for what value?

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java

rskj-core/src/test/java/co/rsk/peg/MinimumPegValueTest.java

marcos-iov

Looks good overall. I think we just need to test for different fee per kb values

rskj-core/src/test/java/co/rsk/peg/MinimumPegValueTest.java

sonarcloud · 2024-08-21T11:49:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

apancorb self-assigned this Jul 23, 2024

github-advanced-security bot found potential problems Jul 23, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Fixed Show fixed Hide fixed

apancorb commented Jul 23, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

apancorb commented Jul 23, 2024

View reviewed changes

apancorb changed the title ~~Write unit tests to showcase the Bridge behaviour under different scenarios~~ Showcase the Bridge behaviour under different scenarios Jul 29, 2024

apancorb marked this pull request as ready for review July 30, 2024 09:40

apancorb requested a review from a team as a code owner July 30, 2024 09:40

marcos-iov reviewed Jul 31, 2024

View reviewed changes

julia-zack reviewed Aug 12, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/ReleaseTransactionBuilderTest.java Outdated Show resolved Hide resolved

apancorb added 2 commits August 15, 2024 10:17

feat(peg): add unit tests for new min pegout value

14234bc

feat(peg): add new test scenario

da54401

apancorb force-pushed the min-pegout-value branch 2 times, most recently from 09c7677 to 0d9b51e Compare August 15, 2024 11:25

apancorb commented Aug 15, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/MinimumPegValueTest.java Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Aug 15, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/MinimumPegValueTest.java Fixed Show fixed Hide fixed

feat(peg): refactor min peg value tests

e47fa7f

apancorb force-pushed the min-pegout-value branch from 0d9b51e to e47fa7f Compare August 15, 2024 11:51

apancorb commented Aug 15, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/MinimumPegValueTest.java Show resolved Hide resolved

marcos-iov reviewed Aug 16, 2024

View reviewed changes

apancorb added 3 commits August 16, 2024 11:14

feat(peg): add different feePerKb values for determining min peg values

41339b3

feat(peg): add minimum peg test for validating release btc log created

782b628

feat(peg): provide more peg minimum parameters

7e2515f

github-advanced-security bot found potential problems Aug 21, 2024

View reviewed changes

rskj-core/src/test/java/co/rsk/peg/MinimumPegValueTest.java Dismissed Show dismissed Hide dismissed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Showcase the Bridge behaviour under different scenarios #2633

Showcase the Bridge behaviour under different scenarios #2633

apancorb commented Jul 23, 2024

apancorb Jul 23, 2024

marcos-iov Jul 31, 2024

apancorb Aug 14, 2024

apancorb Jul 23, 2024

apancorb Jul 29, 2024

marcos-iov Jul 31, 2024

apancorb Aug 14, 2024

marcos-iov left a comment

marcos-iov Jul 31, 2024

apancorb Aug 15, 2024

marcos-iov Jul 31, 2024

marcos-iov Jul 31, 2024

marcos-iov left a comment

sonarcloud bot commented Aug 21, 2024

Showcase the Bridge behaviour under different scenarios #2633

Are you sure you want to change the base?

Showcase the Bridge behaviour under different scenarios #2633

Conversation

apancorb commented Jul 23, 2024

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

marcos-iov left a 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

marcos-iov left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 21, 2024

Quality Gate passed