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

Jump to bottom

Create new Federation and the complete process until it is confirmed and activated #2925

Open
wants to merge 21 commits into
base: federation_change_federeration_integration
Choose a base branch
from
Open

Create new Federation and the complete process until it is confirmed and activated #2925

wants to merge 21 commits into from
+570 −0

Conversation

apancorb
Copy link
Contributor

@apancorb apancorb commented Jan 10, 2025 •
edited
Loading

Description

The idea is to provide building blocks that can be reused while creating new tests. This focuses on creating a new active and retiring federation.

@apancorb apancorb self-assigned this Jan 10, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 10, 2025 •
edited
Loading

Dependency Review

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

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

@apancorb apancorb changed the title Fix/powpeg migration tests common path Create new Federation and the complete process until it is confirmed and activated Jan 10, 2025
@apancorb apancorb marked this pull request as ready for review January 10, 2025 15:33
@apancorb apancorb requested a review from a team as a code owner January 10, 2025 15:33
jeremy-then
jeremy-then reviewed Jan 10, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated
case P2SH_ERP -> {
originalFederation = FederationFactory.buildP2shErpFederation(
originalFederationArgs, erpPubKeys, activationDelay);
// TODO: CHECK REDEEMSCRIPT
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this TODO be here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thanks

jeremy-then
jeremy-then approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@jeremy-then jeremy-then left a comment

Choose a reason for hiding this comment

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

LGTM.

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 1ba5cc7 to 1191212 Compare January 14, 2025 10:48
@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 250c82b to 0f5161b Compare January 15, 2025 11:11
julia-zack
julia-zack approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@julia-zack julia-zack left a comment

Choose a reason for hiding this comment

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

nice work

marcos-iov
marcos-iov reviewed Jan 28, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated
class FederationChangeIT {

private enum FederationType {
NON_STANDARD_ERP,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need this one, the goal is to have all RSKIPs active from the start right?

So we should have genesis federation that is of the standard multisig type, then immediately do a federation change. Now working with a p2sh erp fed. That's the only one we care about.

At least for now, then will come the segwit fed

rskj-core/src/main/java/co/rsk/peg/federation/FederationSupportImpl.java Outdated
@@ -656,7 +656,7 @@ private Integer addFederatorPublicKeyMultikey(boolean dryRun, BtcECKey btcKey, E
* PENDING_FEDERATION_MISMATCHED_HASH if the given hash doesn't match the current pending federation's hash.
* SUCCESSFUL upon success.
*/
private FederationChangeResponseCode commitFederation(boolean dryRun, Keccak256 pendingFederationHash, BridgeEventLogger eventLogger) {
FederationChangeResponseCode commitFederation(boolean dryRun, Keccak256 pendingFederationHash, BridgeEventLogger eventLogger) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we access this method by calling voteFederationChange?

I think we should test this sending the required votes. Make it as realistic as possible. See VoteFederationChangeTest class

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java Outdated
var activations = ActivationConfigsForTest.all().forBlock(0);
setUpFederationChange(activations);
// Create a default original federation using the list of UTXOs
var originalFederation = createOriginalFederation(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we simply get the genesis federation here?

@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch 3 times, most recently from baf74f1 to 89d811f Compare January 30, 2025 10:50
@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 89d811f to dcadcb4 Compare January 30, 2025 12:18
@apancorb apancorb force-pushed the fix/powpeg-migration-tests-common-path branch from 22efa4a to dc6fb3f Compare January 31, 2025 09:16
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 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 SonarQube Cloud

marcos-iov
marcos-iov reviewed Jan 31, 2025
View reviewed changes
rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
var memberRskKey = member.getRskPublicKey();
var memberMstKey = member.getMstPublicKey();

voteToAddFederatorPublicKeysToPendingFederation(memberBtcKey, memberRskKey, memberMstKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps after voting each new member we could get the pending federation and assert that the new member has been added. Along with the previous members.

Something like

assert(pendingFederationSize++)
assert(pendingFederation.hasMember(member)

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
var pendingFederation = federationStorageProvider.getPendingFederation();
assertNotNull(pendingFederation);
// We extract here because we will need for util methods
var newFederation = pendingFederation.buildFederation(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about building it this way here.

I would build the expectedFederation, similar to how we are bullding the originalFederation. Take th public keys + erp constants and use FederationFactory to build the expected federation. That way we don't use the same code we are testing to make the assertions.

Finally, get the active federation from FederationSupport after the commit and compare against expectedFederation. Makes sense?

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
voteToAddFederatorPublicKeysToPendingFederation(NEW_FEDERATION_MEMBERS);

var pendingFederation = federationStorageProvider.getPendingFederation();
assertNotNull(pendingFederation);
Copy link
Contributor

Choose a reason for hiding this comment

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

assert size and members?

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
assertTrue(lastRetiredFederationP2SHScriptOptional.isPresent());
Script lastRetiredFederationP2SHScript = lastRetiredFederationP2SHScriptOptional.get();

if (ACTIVATIONS.isActive(ConsensusRule.RSKIP377)){
Copy link
Contributor

Choose a reason for hiding this comment

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

We expect all RSKIPs to be active

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java

private void assertMigrationHasStarted() throws Exception {
// Pegouts waiting for confirmations should not be empty
assertFalse(bridgeStorageProvider.getPegoutsWaitingForConfirmations().getEntries().isEmpty());
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 check that there is only one element? Should be the case as long as there are less than 50 UTXOs to migrate

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
.map(Entry::getBtcTransaction)
.toList();

if (ACTIVATIONS.isActive(ConsensusRule.RSKIP428)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be active

rskj-core/src/test/java/co/rsk/peg/federation/FederationChangeIT.java
newFederation.getAddress(), originalFederation.getAddress());
assertMigrationHasStarted();
verifySigHashes();
verifyPegoutTransactionCreatedEventWasEmitted();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are other events that should be emitted as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcos-iov marcos-iov marcos-iov left review comments

@jeremy-then jeremy-then jeremy-then approved these changes

@julia-zack julia-zack julia-zack approved these changes

Assignees

@apancorb apancorb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apancorb @marcos-iov @julia-zack @jeremy-then