Add tests validating 32 byte chainwork checkpoints can be processed once RSKIP454 is activated #2865
Conversation
…nce RSKIP454 is activated
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
|
|
||
| InputStream checkpointsStream = bridgeSupport.getCheckPoints(); | ||
| CheckpointManager manager = new CheckpointManager(bridgeTestNetConstants.getBtcParams(), checkpointsStream); | ||
| long time = bridgeSupport.getActiveFederation().getCreationTime().toEpochMilli() - 604800L; // The magic number is a substraction CheckpointManager does when getting the checkpoints. |
There was a problem hiding this comment.
| long time = bridgeSupport.getActiveFederation().getCreationTime().toEpochMilli() - 604800L; // The magic number is a substraction CheckpointManager does when getting the checkpoints. | |
| long checkpointManagerSubstraction = 604800L; | |
| long time = bridgeSupport.getActiveFederation().getCreationTime().toEpochMilli() - checkpointManagerSubstraction; |
wdyt?
| @@ -192,17 +199,114 @@ void testInitialChainHeadWithoutBtcCheckpoints() throws Exception { | |||
| void testInitialChainHeadWithBtcCheckpoints() throws Exception { | |||
| BridgeConstants bridgeTestNetConstants = BridgeTestNetConstants.getInstance(); | |||
|
|
|||
| BridgeSupport bridgeSupport = arrangeBridgeSupport(bridgeTestNetConstants, | |||
There was a problem hiding this comment.
lets add tests for mainnet, wdyt?
There was a problem hiding this comment.
This is the only one testing testnet. The rest of new tests are for mainnnet.
| BridgeSupport bridgeSupport = arrangeBridgeSupport(bridgeTestNetConstants, | ||
| activationsBeforeForks); | ||
|
|
||
| // Force instantiation of blockstore |
There was a problem hiding this comment.
Doing exactly what's being said. Check this:
Basically, we are testing how checkpoints are read and then store in the RepositoryBtcBlockStoreWithCache.
There was a problem hiding this comment.
my concern is: why is this needed? maybe an explanatory comment to add a bit more context
| "12-byte-chainwork-mix-format.production.checkpoints" | ||
| } | ||
| ) | ||
| void testInitialChainHeadWithBtcCheckpoints_whenCheckpointsWith12BytesChainWork_before_RSKIP454_ok( |
There was a problem hiding this comment.
| void testInitialChainHeadWithBtcCheckpoints_whenCheckpointsWith12BytesChainWork_before_RSKIP454_ok( | |
| void initialChainHeadWithBtcCheckpoints_whenCheckpointsWith12BytesChainWork_beforeRSKIP454_ok( |
no need to add test to name a test i think :)
| assertEquals(checkpoint.getHeight(), bridgeSupport.getBtcBlockchainBestChainHeight()); | ||
| } | ||
|
|
||
| private BridgeSupport arrangeBridgeSupport(BridgeConstants bridgeTestNetConstants, |
There was a problem hiding this comment.
not a big fan of this. Can't we set the bridgeSupport in the general setup?
There was a problem hiding this comment.
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.
| "12-byte-chainwork-mix-format.production.checkpoints" | ||
| } | ||
| ) | ||
| void testInitialChainHeadWithBtcCheckpoints_whenCheckpointsWith12BytesChainWork_before_RSKIP454_ok( |
There was a problem hiding this comment.
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
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
Checklist: