Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated #2865

nathanieliov · 2024-11-27T03:28:05Z

Description

Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated

How Has This Been Tested?

Unit tests

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)

…nce RSKIP454 is activated

github-actions · 2024-11-27T03:28:23Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

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

julia-zack · 2024-11-27T11:43:54Z

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

+        BridgeSupport bridgeSupport = arrangeBridgeSupport(bridgeTestNetConstants,
+            activationsBeforeForks);
+
+        // Force instantiation of blockstore


why doing this?

Doing exactly what's being said. Check this:

rskj/rskj-core/src/main/java/co/rsk/peg/BridgeSupport.java

Line 2573 in 8220d12

private StoredBlock getBtcBlockchainChainHead() throws IOException, BlockStoreException {

Basically, we are testing how checkpoints are read and then store in the RepositoryBtcBlockStoreWithCache.

my concern is: why is this needed? maybe an explanatory comment to add a bit more context

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

julia-zack · 2024-11-27T11:50:40Z

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

+        assertEquals(checkpoint.getHeight(), bridgeSupport.getBtcBlockchainBestChainHeight());
+    }
+
+    private BridgeSupport arrangeBridgeSupport(BridgeConstants bridgeTestNetConstants,


not a big fan of this. Can't we set the bridgeSupport in the general setup?

I need to build the bridgeSupport using different bridgeConstants and different activations depending on the use case. This is why I don't put it there, but I'm reorganizing the tests, and let's see if you find it easier to follow.

julia-zack · 2024-11-27T11:52:45Z

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

+            "12-byte-chainwork-mix-format.production.checkpoints"
+        }
+    )
+    void testInitialChainHeadWithBtcCheckpoints_whenCheckpointsWith12BytesChainWork_before_RSKIP454_ok(


can we add the arrange, act, assert sections as we've been doing? I find the tests a bit hard to follow

- Put magical number into meaningful constant variable - Put assertion blocks into methods with meaningful names

apancorb

LGTM

julia-zack · 2024-11-28T15:35:55Z

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


    @BeforeEach
    void setUpOnEachTest() {
-        bridgeConstants = new BridgeRegTestConstants();
+        bridgeConstants = bridgeRegTestConstants;


this is weird, can't we use bridgeRegTestConstants directly?

julia-zack · 2024-11-28T15:46:50Z

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

+        assertEquals(checkpoint.getHeight(), bridgeSupport.getBtcBlockchainBestChainHeight());
+    }
+
+    private void arrangeBridgeSupport() {


Suggested change

private void arrangeBridgeSupport() {

private void arrangeBridgeSupport(bridgeConstants, activations) {

wdyt? to avoid doing the

bridgeConstants = specificConstants activations = specificActivations

btw, i'd set the bridgeSupport in the general setup, and create it again when needed in specific tests.
In general we've been trying to avoid using these type of methods

This whole class needs a refactor for the other methods that are not being touched. But this is out of the scope of this task.

Anyway, regarding using this approach for arrangement in a test, I think you may be referencing the name arrangeBridgeSupport, which could be different. I accept that. But this is the same approach we've been using lately. I got inspired by changes in the svp project. E.g., https://github.com/rsksmart/rskj/blob/add-federation-context/rskj-core/src/test/java/co/rsk/peg/BridgeSupportSvpTest.java#L1148C18-L1148C49

Added your suggestion

jeremy-then

LGTM.

julia-zack · 2024-11-29T13:02:07Z

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

+     * Time to use in CheckpointManager adjust checkpoint backwards by a week to account for possible clock drift in the block headers.
+     * For more detail please see {@link CheckpointManager#checkpoint(NetworkParameters, InputStream, co.rsk.bitcoinj.store.BtcBlockStore, long)}
+     */
+    private static final long checkpointTimeAdjustmentForPossibleCLockDrift = weekInSeconds;


Suggested change

private static final long checkpointTimeAdjustmentForPossibleCLockDrift = weekInSeconds;

private static final long checkpointTimeAdjustmentForPossibleClockDrift = weekInSeconds;

just a little typo

btw, this variable is just being used here.
I'd remove it and use weekInSeconds directly, and move the explanatory comment to where its being used.

julia-zack · 2024-11-29T14:58:49Z

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

@@ -155,88 +174,151 @@ void setUpOnEachTest() {
            LockingCapMainNetConstants.getInstance(),
            signatureCache
        );
+
+        track = createRepository().startTracking();
+        arrangeBridgeSupport(bridgeRegTestConstants, activations);


i insist, don't do this. we don't want a method for arranging the bridge support, that's why we created the bridge support builder.

btw, let's use mainnet for general setup

Mainnet doesn't work for the rest of the tests. Refactoring this test class is out of the scope.

What's your proposal for avoid recreating the bridge support when we need an specific BridgeSupport on certain test cases?

recreate it inside the test. Do not create an arrangeBridgeSupport method.

Done. Got rid of arrangeBridgeSupport.

julia-zack · 2024-11-29T15:02:06Z

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

-            .build();
+        // arrange
+        activations = activationsBeforeForks;
+        bridgeRegTestConstants = bridgeRegTestConstants;


julia-zack · 2024-11-29T15:02:43Z

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

+        activations = activationsBeforeForks;
+        bridgeRegTestConstants = bridgeRegTestConstants;
+        arrangeBridgeSupport(bridgeRegTestConstants, activations);


Suggested change

activations = activationsBeforeForks;

bridgeRegTestConstants = bridgeRegTestConstants;

arrangeBridgeSupport(bridgeRegTestConstants, activations);

arrangeBridgeSupport(bridgeRegTestConstants, activationsBeforeForks);

Add tests validating 32 byte chainwork checkpoints can be processed o…

60bf54a

…nce RSKIP454 is activated

nathanieliov requested a review from a team as a code owner November 27, 2024 03:28

julia-zack reviewed Nov 27, 2024

View reviewed changes

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

julia-zack reviewed Nov 27, 2024

View reviewed changes

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

julia-zack reviewed Nov 27, 2024

View reviewed changes

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

julia-zack reviewed Nov 27, 2024

View reviewed changes

- Reorganize tests

cc9ec1a

- Put magical number into meaningful constant variable - Put assertion blocks into methods with meaningful names

apancorb approved these changes Nov 28, 2024

View reviewed changes

julia-zack reviewed Nov 28, 2024

View reviewed changes

jeremy-then approved these changes Nov 29, 2024

View reviewed changes

julia-zack reviewed Nov 29, 2024

View reviewed changes

nathanieliov force-pushed the add-textual-checkpoints-support branch 2 times, most recently from 69feb2c to 7db7156 Compare November 29, 2024 19:21

Reorganize tests

57b1a16

nathanieliov force-pushed the add-textual-checkpoints-support branch from 7db7156 to 57b1a16 Compare November 29, 2024 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated #2865

Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated #2865

nathanieliov commented Nov 27, 2024

github-actions bot commented Nov 27, 2024 •

edited

Loading

julia-zack Nov 27, 2024

nathanieliov Nov 27, 2024

julia-zack Nov 27, 2024

nathanieliov Nov 27, 2024

julia-zack Nov 27, 2024

nathanieliov Nov 27, 2024

nathanieliov Nov 27, 2024

julia-zack Nov 27, 2024

nathanieliov Nov 27, 2024

apancorb left a comment

julia-zack Nov 28, 2024

nathanieliov Nov 29, 2024

julia-zack Nov 28, 2024

julia-zack Nov 28, 2024

nathanieliov Nov 29, 2024

nathanieliov Nov 29, 2024

jeremy-then left a comment

julia-zack Nov 29, 2024

julia-zack Nov 29, 2024

nathanieliov Nov 29, 2024

julia-zack Nov 29, 2024

julia-zack Nov 29, 2024 •

edited

Loading

nathanieliov Nov 29, 2024

nathanieliov Nov 29, 2024

julia-zack Nov 29, 2024

nathanieliov Nov 29, 2024 •

edited

Loading

julia-zack Nov 29, 2024

nathanieliov Nov 29, 2024

julia-zack Nov 29, 2024

nathanieliov Nov 29, 2024

	private void arrangeBridgeSupport() {
	private void arrangeBridgeSupport(bridgeConstants, activations) {

	private static final long checkpointTimeAdjustmentForPossibleCLockDrift = weekInSeconds;
	private static final long checkpointTimeAdjustmentForPossibleClockDrift = weekInSeconds;

Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated #2865

Are you sure you want to change the base?

Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated #2865

Conversation

nathanieliov commented Nov 27, 2024

Description

How Has This Been Tested?

Types of changes

Checklist:

github-actions bot commented Nov 27, 2024 • edited Loading

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

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

apancorb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy-then 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

julia-zack Nov 29, 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

nathanieliov Nov 29, 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

github-actions bot commented Nov 27, 2024 •

edited

Loading

julia-zack Nov 29, 2024 •

edited

Loading

nathanieliov Nov 29, 2024 •

edited

Loading